Skip to content
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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Oct 31, 2024

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:

  1. wallet_CreateMultiTransaction and wallet_ProceedWithTransactionsSignatures are now deprecated and not used anymore.
  2. wallet.sign.transactions signal is deprecated and not used either.

The new transaction flow looks like this:

  • we call wallet_BuildTransactionsFromRoute
  • wait for the wallet.router.sign-transactions signal
  • sign received hashes using wallet_SignMessage call or sign on keycard
  • call wallet_SendRouterTransactionsWithSignatures with the signatures of signed hashes from the previous step
  • wallet.router.transactions-sent signal will be sent after sending transactions or an error occurs

Platforms

  • Android
  • iOS

Areas that maybe impacted

Functional
  • swaps
  • send transactions

@alwx alwx self-assigned this Oct 31, 2024
@status-im-auto
Copy link
Member

status-im-auto commented Oct 31, 2024

Jenkins Builds

Click to see older builds (40)
Commit #️⃣ Finished (UTC) Duration Platform Result
cf5e396 #1 2024-10-31 12:12:07 ~2 min tests 📄log
✔️ cf5e396 #1 2024-10-31 12:17:17 ~7 min android-e2e 🤖apk 📲
✔️ cf5e396 #1 2024-10-31 12:18:16 ~8 min android 🤖apk 📲
✔️ cf5e396 #1 2024-10-31 12:19:05 ~9 min ios 📱ipa 📲
840b9dc #2 2024-10-31 14:47:07 ~3 min tests 📄log
dcfdfe2 #4 2024-10-31 14:53:34 ~2 min tests 📄log
✔️ dcfdfe2 #4 2024-10-31 14:58:57 ~7 min android-e2e 🤖apk 📲
✔️ dcfdfe2 #4 2024-10-31 14:59:53 ~8 min android 🤖apk 📲
✔️ dcfdfe2 #4 2024-10-31 15:00:18 ~9 min ios 📱ipa 📲
0d03673 #6 2024-11-04 09:04:22 ~2 min tests 📄log
✔️ 0d03673 #6 2024-11-04 09:09:39 ~7 min android-e2e 🤖apk 📲
✔️ 0d03673 #6 2024-11-04 09:09:54 ~8 min android 🤖apk 📲
✔️ 0d03673 #6 2024-11-04 09:10:39 ~8 min ios 📱ipa 📲
a356269 #7 2024-11-05 11:41:39 ~2 min tests 📄log
✔️ a356269 #7 2024-11-05 11:47:10 ~8 min android-e2e 🤖apk 📲
✔️ a356269 #7 2024-11-05 11:47:49 ~9 min ios 📱ipa 📲
✔️ a356269 #7 2024-11-05 11:47:55 ~9 min android 🤖apk 📲
af212ff #8 2024-11-06 12:47:16 ~2 min tests 📄log
af212ff #8 2024-11-06 12:47:29 ~3 min ios 📄log
✔️ af212ff #8 2024-11-06 12:52:55 ~8 min android-e2e 🤖apk 📲
✔️ af212ff #8 2024-11-06 12:53:30 ~9 min android 🤖apk 📲
b86660d #9 2024-11-06 16:08:47 ~2 min tests 📄log
b86660d #9 2024-11-06 16:09:07 ~2 min ios 📄log
✔️ b86660d #9 2024-11-06 16:15:42 ~9 min android-e2e 🤖apk 📲
✔️ b86660d #9 2024-11-06 16:16:12 ~9 min android 🤖apk 📲
d1afb9f #10 2024-11-06 22:09:14 ~2 min ios 📄log
d1afb9f #10 2024-11-06 22:09:35 ~3 min tests 📄log
1a5e40a #11 2024-11-06 22:14:12 ~2 min ios 📄log
1a5e40a #11 2024-11-06 22:15:00 ~3 min tests 📄log
✔️ 1a5e40a #11 2024-11-06 22:19:41 ~7 min android-e2e 🤖apk 📲
✔️ 1a5e40a #11 2024-11-06 22:20:14 ~8 min android 🤖apk 📲
78f1308 #12 2024-11-06 22:27:16 ~2 min tests 📄log
039ac63 #13 2024-11-06 22:29:58 ~2 min ios 📄log
039ac63 #13 2024-11-06 22:30:08 ~2 min tests 📄log
✔️ 039ac63 #13 2024-11-06 22:35:10 ~7 min android-e2e 🤖apk 📲
✔️ 039ac63 #13 2024-11-06 22:35:38 ~8 min android 🤖apk 📲
3079210 #15 2024-11-08 14:29:14 ~3 min ios 📄log
3079210 #15 2024-11-08 14:29:25 ~3 min tests 📄log
✔️ 3079210 #15 2024-11-08 14:34:22 ~8 min android-e2e 🤖apk 📲
✔️ 3079210 #15 2024-11-08 14:36:09 ~10 min android 🤖apk 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
efff9d8 #16 2024-11-11 09:10:33 ~4 min tests 📄log
✔️ efff9d8 #16 2024-11-11 09:14:53 ~9 min ios 📱ipa 📲
✔️ efff9d8 #16 2024-11-11 09:15:41 ~9 min android-e2e 🤖apk 📲
✔️ efff9d8 #16 2024-11-11 09:17:04 ~11 min android 🤖apk 📲
6422569 #17 2024-11-12 13:10:04 ~3 min tests 📄log
✔️ 6422569 #17 2024-11-12 13:15:22 ~8 min android 🤖apk 📲
✔️ 6422569 #17 2024-11-12 13:15:50 ~8 min ios 📱ipa 📲
✔️ 6422569 #17 2024-11-12 13:15:57 ~9 min android-e2e 🤖apk 📲

@alwx alwx force-pushed the update-sending-transactions-endpoint branch from c2051ca to 0d03673 Compare November 4, 2024 09:01
@alwx alwx force-pushed the update-sending-transactions-endpoint branch from 1a5e40a to 78f1308 Compare November 6, 2024 22:24
@alwx alwx changed the title WIP: Update sending transaction end point Update sending transaction end point Nov 6, 2024
@alwx alwx changed the title Update sending transaction end point Update sending transaction end point (#21480 + #21555) Nov 6, 2024
@alwx alwx marked this pull request as ready for review November 6, 2024 22:24
@alwx
Copy link
Contributor Author

alwx commented Nov 6, 2024

This one is now ready to be reviewed.

Comment on lines +263 to +267
(defn signature-rsv
[signature]
{:r (subs signature 0 64)
:s (subs signature 64 128)
:v (subs signature 128 130)})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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.

Copy link
Contributor Author

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).

Copy link
Member

@clauxx clauxx Nov 8, 2024

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

@pavloburykh
Copy link
Contributor

@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 alwx changed the title Update sending transaction end point (#21480 + #21555) Update sending transaction end point (#21480) Nov 7, 2024
@alwx alwx requested a review from flexsurfer November 7, 2024 16:08
@smohamedjavid
Copy link
Member

@alwx - Can you help me update this PR to point to status-im/status-go#6059 PR (fix/swap-bridge-sending branch)?

It requires the new send flow #21600 (comment)

@pavloburykh
Copy link
Contributor

@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.

@alwx alwx force-pushed the update-sending-transactions-endpoint branch from 039ac63 to 9e94347 Compare November 8, 2024 14:23
@alwx
Copy link
Contributor Author

alwx commented Nov 8, 2024

@pavloburykh done!

@VolodLytvynenko VolodLytvynenko self-assigned this Nov 8, 2024
@alwx
Copy link
Contributor Author

alwx commented Nov 10, 2024

@status-im/wallet-mobile-devs I need some reviews for this

@alwx alwx requested a review from vkjr November 10, 2024 12:58
@VolodLytvynenko
Copy link
Contributor

VolodLytvynenko commented Nov 11, 2024

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 swapped

Steps:

  1. Go to swap page
  2. Swap any asset

Actual result:

"Price route not found error is shown"
image

Expected result:

Swap transaction is successful

Mobile logs

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.

Copy link
Contributor

@mohsen-ghafouri mohsen-ghafouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@VolodLytvynenko
Copy link
Contributor

@saledjenic Additional info: this issue #21541 (comment) happens for ETH assets as well. however on the last nightly ETH assets can be swapped

@VolodLytvynenko
Copy link
Contributor

Bridge erc20 transactions are fixed! @alwx @saledjenic thank you!

@pavloburykh
Copy link
Contributor

pavloburykh commented Nov 11, 2024

hi @saledjenic Issue with the inability to swap ERC-20 is still reproducible in the current PR after the Status Go update

The bug might also be related to the issues within new end point implementation, so it is worth checking from @alwx side as well.

Copy link
Member

@briansztamfater briansztamfater left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: CONTRIBUTOR
Development

Successfully merging this pull request may close these issues.

ERC20 bridge and swap transactions are broken 📤 Update sending transaction end point
8 participants