feat_: sending transaction improvements #5807
Conversation
Jenkins BuildsClick to see older builds (104)
|
saledjenic
force-pushed
the
transaction-improvements
branch
from
89425ce
to
45ace27
Compare
saledjenic
force-pushed
the
router-clean-up
branch
from
557c4f7
to
cbb74b7
Compare
saledjenic
force-pushed
the
transaction-improvements
branch
from
45ace27
to
e84de17
Compare
services/wallet/api.go
Outdated
| @@ -742,11 +753,91 @@ func (api *API) CreateMultiTransaction(ctx context.Context, multiTransactionComm | |||
| return nil, api.s.transactionManager.SendTransactionForSigningToKeycard(ctx, cmd, data, api.router.GetPathProcessors()) | |||
| } | |||
|
|
|||
| func (api *API) BuildTransactionsFromRoute(ctx context.Context, uuid string, slippagePercentage float32) (err error) { | |||
There was a problem hiding this comment.
we can make this API a bit more break-proof by using a struct (parameters or something) instead of plainly slippagePercentage
There was a problem hiding this comment.
@dlipicar agree, good idea, done.
| @@ -55,9 +64,17 @@ type SendTxArgs struct { | |||
| Input types.HexBytes `json:"input"` | |||
| Data types.HexBytes `json:"data"` | |||
|
|
|||
| // additional data | |||
| // additional data - version SendTxArgsVersion1 | |||
There was a problem hiding this comment.
hmm not sure why we decided to just copy all the fields from ethapi.TransactionArgs (raw parameters needed to send a tx to the chain), I kind of like to keep that as a separate struct and then have all the remaining "metadata" as additional fields. Then the lower level modules that are not interested in the metadata can just consume the original type.
There was a problem hiding this comment.
@dlipicar this is the first step of improvements and simplifications I want to do, we can discuss that for some of the next steps for sure, but this was the easiest now since any bigger change will require updating Transactor that I don't want to touch yet, since I didn't want to break any endpoint that we already have and is being used by the mobile app.
transactions/types.go
Outdated
| ToChainID uint64 | ||
| FromTokenID string | ||
| ToTokenID string | ||
| ToContractAddress types.Address |
There was a problem hiding this comment.
it's not clear what this parameter's meaning is without looking at the code, could you add some comment?
There was a problem hiding this comment.
@dlipicar comment added.
saledjenic
force-pushed
the
router-clean-up
branch
from
cbb74b7
to
900ad9b
Compare
saledjenic
force-pushed
the
transaction-improvements
branch
from
e84de17
to
c4dc7d7
Compare
saledjenic
force-pushed
the
transaction-improvements
branch
4 times, most recently
from
5c31f55
to
c515d9d
Compare
| const ( | ||
| SendTxArgsVersion0 SendTxArgsVersion = 0 | ||
| SendTxArgsVersion1 SendTxArgsVersion = 1 | ||
| ) |
There was a problem hiding this comment.
I guess this version will go away when deprecated code will be handle, maybe add a comment to remove this Version attribute?
There was a problem hiding this comment.
Sure, will add. And yes, once the mobile app switches to the new tx sending flow, and we make sure that there is no part of the app that uses old sendargs, we can simplify Transactor a lot and remove the version property.
Once we move all TXs sendings to this flow, we will have a uniform flow across the app that is easier to maintain and extend.
saledjenic
requested review from
shivekkhurana,
briansztamfater and
alexjba
saledjenic
force-pushed
the
transaction-improvements
branch
from
c515d9d
to
0506f56
Compare
There was a problem hiding this comment.
@saledjenic I would like to open a discussion here and bring some attention.
This PR is too big.
And there're 2 issues behind such huge PRs. Even Github files diff barely loads 🫠
-
Reviewers waste too much time and energy.
And if they don't it means they didn't pay enough attention.
If I was to review such PR carefully, it would take 1-2 hours of high concentration, just to keep all the details in mind. -
The chances to spot a bug are very low.
Rarely can a developer keep so much stuff in mind. And even more rare they can assume how such code would break.
This basically makes the reviews useless.I admire you as a dev, the amount of stuff you can keep in mind is great. But other developers are not like you.
-
With due respect, there's no way even you can think of all the edge cases
We're all humans and we do mistakes. And there're 2 ways to fight this: code review and testing. Code review is useless in such case. And there're no tests either.
-
There are no tests
You know, me and you, we love this topic ❤️ 😄
I see that you opened a separate ticket. But why we can't implement it in a same PR?
We have no rush now. There's no milestone. There's no deadline. The focus is quality and reliability. Isn't it?
Also, such PRs take too much time to merge. And meanwhile you start preparing new changes on top of these. And then the PR is pushed to be reviewed and tested ASAP, as it's been open for so long, and there's another portion of code waiting.
This process is unhealthy for the product and everyone involved.
It would go much faster and simpler for everyone, if the iterations were more granular.
Having this said,
Would you please consider dividing this PR into a few smaller ones?
|
@igor-sirotin I appreciate your perspective and what you brought up, and I agree with most of them, but some things I see a bit differently. :) First of all, I don't know why you are for smaller PRs instead of having multiple logically split commits in a single PR?
I can organize them in several commits instead, not PRs, hope that works?
The size of the PR depends on the change that is done. So if I want to rename a single function that is used in many places, I will have many changes and the PR will be big, but that change cannot be split. This may be a bad example, but what I want to say is I added a new function
Correct, there are no tests here, yet, at the time of this PR, the mechanism for handling signals for functional tests was not implemented (not sure if that's merged even today), but because of that, I informed Anton D about that and created a new issue for adding tests for this part #5831 that was mentioned in the description of this PR. |
|
@saledjenic thank you for being open for discussion ❤️ Large PR split by commits VS. several small PRs
TestsSoo, we actually have a policy in the repo: status-go/_docs/policies/tests.md Line 8 in f04a9a8 It's totally fine if @antdanchenko implements the tests. But they should be introduced in the same PR, because we agreed on the intention to keep
FYI, it was merged a week ago 👌 Really, we're discussing things here that are a well-known good practices 🤔 |
saledjenic
force-pushed
the
transaction-improvements
branch
2 times, most recently
from
89503c9
to
88b11d7
Compare
saledjenic
force-pushed
the
transaction-improvements
branch
3 times, most recently
from
454d0f5
to
b1575d0
Compare
|
@igor-sirotin all your comments are addressed, thanks for the review. |
✔️ status-go/prs/tests/PR-5807#22 🔹 ~31 min 🔹 b1575d0 🔹 📦 tests package |
|
@saledjenic as just today we merged all swaps related PRs + all fixes after last wallet refactoring, we'll retest it briefly one more time, thank you! |
Based on changes done in this PR status-im/status-go#5807 we can simplify our client logic a lot. This results in the removal of many lines of code that are no longer needed Closes 1st part of #16336
|
Hey @saledjenic , thanks for the PR! I’ve checked to see if it affects mobile, and I can confirm it doesn’t while also fixing the ERC-20 approval issues on Arbitrum. Please let me know if any last commits are added, and I'll quickly recheck to ensure it still doesn't impact mobile. I promise it won't take long :) |
|
Thanks for checking @VolodLytvynenko. Nothing new was added, since I just rebase this PR from time to time, cause it's opened for a long time already and I have other PRs based on it and that's why you see/receive email about changes. |
Based on changes done in this PR status-im/status-go#5807 we can simplify our client logic a lot. This results in the removal of many lines of code that are no longer needed Closes 1st part of #16336
Added props: - `Version` used to differ old and new usage - `ValueOut` - `FromChainID` - `ToChainID` - `FromTokenID` - `ToTokenID` - `ToContractAddress` - `SlippagePercentage`
…cess This commit simplifies the sending process of the best route suggested by the router. It also makes the sending process the same for accounts (key pairs) migrated to a keycard and those stored locally in local keystore files. Deprecated endpoints: - `CreateMultiTransaction` - `ProceedWithTransactionsSignatures` Deprecated signal: - `wallet.sign.transactions` New endpoints: - `BuildTransactionsFromRoute` - `SendRouterTransactionsWithSignatures` The flow for sending the best router suggested by the router: - call `BuildTransactionsFromRoute` - wait for the `wallet.router.sign-transactions` signal - sign received hashes using `SignMessage` call or sign on keycard - call `SendRouterTransactionsWithSignatures` with the signatures of signed hashes from the previous step - `wallet.router.transactions-sent` signal will be sent after transactions are sent or if an error occurs New signals: - `wallet.router.sending-transactions-started` // notifies client that the sending transactions process started - `wallet.router.sign-transactions` // notifies client about the list of transactions that need to be signed - `wallet.router.transactions-sent` // notifies client about transactions that are sent - `wallet.transaction.status-changed` // notifies about status of sent transactions
saledjenic
force-pushed
the
transaction-improvements
branch
from
b1575d0
to
4773c62
Compare
There was a problem hiding this comment.
The code's good, thanks @saledjenic for the fixes.
I still see a lot of uncovered new code, which goes against the agreed policy.
I won't hold this PR, it's been around for 1 month already. But I'm worried about this, so please let's discuss this and agree on our processes for the new PRs.
Based on changes done in this PR status-im/status-go#5807 we can simplify our client logic a lot. This results in the removal of many lines of code that are no longer needed Closes 1st part of #16336
Based on changes done in this PR status-im/status-go#5807 we can simplify our client logic a lot. This results in the removal of many lines of code that are no longer needed Closes 1st part of #16336
Based on changes done in this PR status-im/status-go#5807 we can simplify our client logic a lot. This results in the removal of many lines of code that are no longer needed Closes 1st part of #16336
Based on changes done in this PR status-im/status-go#5807 we can simplify our client logic a lot. This results in the removal of many lines of code that are no longer needed Closes 2nd part of #16336
…ing flow Based on changes done in this PR status-im/status-go#5807 we can simplify our client logic a lot. This results in the removal of many lines of code that are no longer needed Closes 2nd part of #16336
…ing flow Based on changes done in this PR status-im/status-go#5807 we can simplify our client logic a lot. This results in the removal of many lines of code that are no longer needed Closes 2nd part of #16336
Instead of the complicated approach we used before, these changes should simplify sending transaction/s suggested by the Router. Many calculations that were done on the client side before will be fully handled on the statusgo side. These changes make the sending process the same for accounts (key pairs) migrated to a keycard and those stored locally in local keystore files.
Deprecated endpoints:
CreateMultiTransactionProceedWithTransactionsSignaturesDeprecated signal:
wallet.sign.transactionsNew endpoints:
BuildTransactionsFromRouteSendRouterTransactionsWithSignaturesThe flow for sending the best router suggested by the router:
BuildTransactionsFromRoutewallet.router.sign-transactionssignalSignMessagecall or sign on keycardSendRouterTransactionsWithSignatureswith the signatures of signed hashes from the previous stepwallet.router.transactions-sentsignal will be sent after sending transactions or an error occursNew signals:
wallet.router.sending-transactions-started// notifies client that the sending transactions process startedwallet.router.sign-transactions// notifies client about the list of transactions that need to be signedwallet.router.transactions-sent// notifies client about transactions that are sentwallet.transaction.status-changed// notifies about status of sent transactionsTests for this PR will be added via this issue: