feat: add ability to send erc20 tokens #18481
Conversation
briansztamfater
requested review from
shivekkhurana,
rasom,
vkjr,
smohamedjavid,
J-Son89,
OmarBasem,
mmilad75 and
ulisesmac
briansztamfater
force-pushed
the
feat/send-erc20-tokens
branch
from
0d8ed3a to
6d9da65
Compare
Jenkins Builds
|
85% of end-end tests have passedFailed tests (4)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (41)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
| gas-fees]} route | ||
| eip-1559-enabled? (:eip-1559-enabled gas-fees) | ||
| {:keys [gas-price max-fee-per-gas-medium | ||
| max-priority-fee-per-gas]} gas-fees | ||
| transfer-tx (cond-> {:From from-address | ||
| :To (or token-address to-address) | ||
| :Gas (money/to-hex gas-amount) | ||
| :Value amount-out | ||
| :Value (when eth-transfer? amount-out) |
There was a problem hiding this comment.
is eth-transfer specific to eth? or should this be token-transfer?
There was a problem hiding this comment.
Yes, it is specific to ETH (or native token), for the rest of tokens we use erc20-transfer?
| :to-address to-address | ||
| :from-asset token-id | ||
| :to-asset token-id | ||
| :amount-out (if eth-transfer? (:amount-out route) "0x0")}) |
There was a problem hiding this comment.
perhaps "0x0" should be a named const? e.g one-in-hex
|
Hey @briansztamfater! Thanx for the PR. One question: I see that this PR is based on feat/send-collectible-events-4 branch, which has not been tested yet and in theory it might have changes/fixes in case the bugs are found. Maybe we should better combine these features into one PR to test them in one common branch? WDYT? |
In case if this is not a good idea, I guess we better test (https://github.com/status-im/status-mobile/tree/feat/send-collectible-events-4) first and after it will be merged we should base current PR brach against the develop. |
Hey @pavloburykh ! I have no strong opinion on this, I can adapt to whatever is better for you. The base PR contains major changes and as they are modifying the same part of the code I decided to use that branch as base, to avoid conflicts when merging it |
We are currently kinda blocked with testing https://github.com/status-im/status-mobile/tree/feat/send-collectible-events-4 because we don't have a good way to get collectibles on testnet. Therefore we cannot test sending properly. So we can test current PR with sending erc20 tokens but as far as understand we cannot merge it until https://github.com/status-im/status-mobile/tree/feat/send-collectible-events-4 is tested. |
|
@briansztamfater Thank you for PR. Here is a found issue: ISSUE 1: [Android] The buttons on sending page does not respondsSteps:
Actual result:'done' or [x] buttons does not close the 'sending' page Untitled.mp4Expected result:After 'done' or [x] buttons are tapped the 'sending' page should be closed ENV:Real device: Pixel 7a, Android 14 Logs: |
That is a separate issue and I have addressed in a pr I opened today 👌 |
briansztamfater
force-pushed
the
feat/send-collectible-events-4
branch
from
d655588 to
4cc796a
Compare
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
feat/send-erc20-tokens
branch
from
6d9da65 to
60ce2c7
Compare
fixes #18413
Summary
As a continuation of #18473 which adds support for sending ERC721 tokens, this PR enables sending ERC20 tokens, for example STT in Goerli, or SNT in mainnet.
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready