fix(wallet)_: enrich status changed and tx update signals' payload with tx route data #6050
Conversation
Jenkins BuildsClick to see older builds (81)
|
saledjenic
changed the title
saledjenic
force-pushed
the
fix/tx-signals-enrich-data
branch
from
1a92352
to
9f0261c
Compare
saledjenic
changed the title
saledjenic
requested review from
alaibe,
shivekkhurana,
dlipicar,
briansztamfater and
smohamedjavid
| import "github.com/status-im/status-go/errors" | ||
|
|
||
| var ( | ||
| ErrInvalidSignatureDetails = &errors.ErrorResponse{Code: errors.ErrorCode("WT-004"), Details: "invalid signature details"} |
There was a problem hiding this comment.
errors of a given type should all be in the same file IMO, hard to track down which error code I need to use for the next one otherwise. If this was causing a circular dependency we might need a separate errors package under the router dir or wherever this used to be
| UsdcSymbol = "USDC" | ||
| HopSymbol = "HOP" | ||
|
|
||
| ProcessorTransferName = "Transfer" |
There was a problem hiding this comment.
hmm these don't really sound like a "common" thing, it's a very pathprocessor-specific thing. I'd favour doing a new package like pathprocessortypes or something like that if it casuses circular dependency, otherwise the common package will just become a dump of every constant used in more than one place.
saledjenic
force-pushed
the
fix/tx-signals-enrich-data
branch
from
9f0261c
to
4a6f483
Compare
|
@dlipicar I agree with both things you've stated above, but was very hard to fix all cyclic dependencies, cause there were many, without such changes. We should think about and work more on sorting out the code organization for the wallet service. |
|
Changes I've done were necessary to have what I need for ephemeral notifications, which is the last commit in this PR. If you can apply the last commit on top of your changes done here #5970 solving all cyclic dependencies with better code organization I am up for that. We should have a separate PR where we will clean the wallet service and improve its architecture. Also, because of many things that are potentially used by mobile app, we cannot fully clean them. I hope things will get better once the mobile app starts using a new sending flow and all txs (for ens usernames, collectibles, stickers, other txs) go over the router and new sending flow. |
dlipicar
force-pushed
the
feat/store-route-execution-data
branch
3 times, most recently
from
2b16ad1
to
ddfcc79
Compare
saledjenic
force-pushed
the
fix/tx-signals-enrich-data
branch
from
4a6f483
to
a30cc4c
Compare
saledjenic
force-pushed
the
fix/tx-signals-enrich-data
branch
2 times, most recently
from
fc6637e
to
ff68921
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6050 +/- ##
============================================
- Coverage 60.87% 13.33% -47.55%
============================================
Files 811 800 -11
Lines 108894 107756 -1138
============================================
- Hits 66293 14371 -51922
- Misses 34843 91533 +56690
+ Partials 7758 1852 -5906
Flags with carried forward coverage won't be shown. Click here to find out more.
|
saledjenic
force-pushed
the
fix/tx-signals-enrich-data
branch
from
ff68921
to
5786466
Compare
| Hash eth.Hash `json:"hash"` | ||
| ChainID common.ChainID `json:"chainId"` | ||
| Hash eth.Hash `json:"hash"` | ||
| SendDetails *responses.SendDetails `json:"sendDetails"` |
There was a problem hiding this comment.
Not good. TxIdentity is supposed to be a very generic super light struct used to uniquely identify a transaction (so, chainID and hash). And PendingTxTracker is supposed to be a generic and single-responsibility (though it's more complex than it should be) Tx status tracker. I'd rather keep it that way and not add a bunch of data to the type/dependencies to the tracker.
IMO, if the client needs more data, better add some API endpoint like "GetRouteDetailsForTxIdentity" and have the client fetch it. Otherwise, create some other event type with this chunky payload and emit it separately in a more appropriate place.
There was a problem hiding this comment.
There is no use case where TxIdentity was sent as a payload of a signal, but I agree, based on the type name it's too much for that type, that's why I added a new type. Also, wherever we want to update the client about the tx change, we need to provide the full context (not only txHash and chainId) so that the client knows what change occurred actually and what to do.
To avoid confusion, I moved SendDetails and SentTransactions to a new TxDetails type that both PendingTxUpdatePayload and StatusChangedPayload have now.
saledjenic
force-pushed
the
fix/tx-signals-enrich-data
branch
2 times, most recently
from
f19ea27
to
770c03e
Compare
New EnsResolver type identified and will be responsible for network calls, while ens api will use it (until mobile app switches to a new sending flow) and continue managing other (db) interactions.
- path processor constants moved to wallet constants - FetchPrices and FindToken functions moved from send type package to router package
saledjenic
force-pushed
the
fix/tx-signals-enrich-data
branch
from
770c03e
to
7d288a7
Compare
…ith tx route data
saledjenic
force-pushed
the
fix/tx-signals-enrich-data
branch
from
7d288a7
to
4e537e8
Compare
As can be seen, by commits, all but the last commit were necessary to get rid of cyclic dependencies I ran into when I wanted to add route tx data to the payload of the status-changed signal.
What's done:
SendTxArgstype defined and used across many packages) moved to services/wallet/wallettypesThese changes depend on the work done here #5970