-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update sending transaction end point (#21480) #21541
base: develop
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (40)
|
c2051ca
to
0d03673
Compare
1a5e40a
to
78f1308
Compare
This one is now ready to be reviewed. |
(defn signature-rsv | ||
[signature] | ||
{:r (subs signature 0 64) | ||
:s (subs signature 64 128) | ||
:v (subs signature 128 130)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated functions here?
https://github.com/status-im/status-mobile/pull/21589/files#r1831380457
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see you are removing that file in this PR. Better to coordinate with @clauxx the path forward for this so we don't waste QA time testing the same code twice (also solving rebase conflicts better).
As I mentioned in Cristian's PR, would love to see some unit tests for this function, so it can serve as documentation for this sensible wallet code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. I removed that one.
This one is fixed + it returns only the signature, and not the vector containing the tx-hash (which is kinda dumb in the original one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that PR was already QA'd, so I'll just merge it since this PR might take longer to merge. Conflict-wise shouldn't make much of a difference
@alwx UPDATE: your PR does not fix #21555. Most likely your branch was outdated when I checked, that's why I didn't reproduce. But I am reproducing now in the latest rebased build. Issue should be fixed by status-im/status-go#6059 |
@alwx - Can you help me update this PR to point to status-im/status-go#6059 PR ( It requires the new send flow #21600 (comment) |
@alwx could you update go version in your PR #21541 so it uses latest go develop? Sale has merged this PR status-im/status-go#6059 and we need to test in scope of your PR. |
039ac63
to
9e94347
Compare
@pavloburykh done! |
@status-im/wallet-mobile-devs I need some reviews for this |
hi @saledjenic Issue with the inability to swap ERC-20 is still reproducible in the current PR after the Status Go update ISSUE 1: ERC-20 and ETH can't be swappedSteps:
Actual result:"Price route not found error is shown" Expected result:Swap transaction is successful Additional info:Swap transactions have been fixed in the Status Go PR: status-im/status-go#6059, which I’ve just verified in the following Desktop PR: status-im/status-desktop#16745. It seems this issue is only occurring on the mobile side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
@saledjenic Additional info: this issue #21541 (comment) happens for ETH assets as well. however on the last nightly ETH assets can be swapped |
Bridge erc20 transactions are fixed! @alwx @saledjenic thank you! |
The bug might also be related to the issues within new end point implementation, so it is worth checking from @alwx side as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Fixes #21480
In scope of this PR we need to test go PR status-im/status-go#6059 which should fix #21555 (QA Note: before testing make sure current PR is pointing to the latest go develop so includes this fix status-im/status-go#6059 which has been already merged)
Basically utilizes the new transaction flow implemented on the go side:
wallet_CreateMultiTransaction
andwallet_ProceedWithTransactionsSignatures
are now deprecated and not used anymore.wallet.sign.transactions
signal is deprecated and not used either.The new transaction flow looks like this:
wallet_BuildTransactionsFromRoute
wallet.router.sign-transactions
signalwallet_SignMessage
call or sign on keycardwallet_SendRouterTransactionsWithSignatures
with the signatures of signed hashes from the previous stepwallet.router.transactions-sent
signal will be sent after sending transactions or an error occursPlatforms
Areas that maybe impacted
Functional