fix: reintroduce changes for sending erc20 and erc721 tokens #18679
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (17)
|
briansztamfater
force-pushed
the
fix/reintroduce-collectibles-erc20
branch
2 times, most recently
from
36b9bf3
to
af9d874
Compare
briansztamfater
requested review from
OmarBasem,
smohamedjavid,
J-Son89,
mmilad75,
ulisesmac,
shivekkhurana,
rasom and
vkjr
briansztamfater
changed the title
smohamedjavid
approved these changes
Jan 31, 2024
| token-id (if token | ||
| (:symbol token) | ||
| (get-in collectible [:id :token-id])) | ||
| erc20-transfer? (and token (not= token-id "ETH")) |
There was a problem hiding this comment.
Should we move "ETH" to a constant for better maintainability?
briansztamfater
force-pushed
the
fix/reintroduce-collectibles-erc20
branch
from
af9d874
to
04c3b16
Compare
|
Hi @briansztamfater! We have recently merged PR by @smohamedjavid which fixed the issue with routes. Do we still need to merge this PR as well? |
ulisesmac
approved these changes
Jan 31, 2024
briansztamfater
force-pushed
the
fix/reintroduce-collectibles-erc20
branch
from
04c3b16
to
db63810
Compare
Hi @VolodLytvynenko! Yes, I was not sure which issue to link, this PR reintroduces the ability to send collectibles and ERC20 tokens, so we would be able to complete a send flow with ETH, ERC20 token and collectibles when we merge this PR.
@ulisesmac great to hear that, I was wondering if you cherry-picked this commit for the demo 😃 |
|
Hi @briansztamfater, thanks for the PR. I've successfully sent collectibles and tokens when the testnet mode toggle is enabled. However, when the testnet mode is disabled, I'm unable to send collectibles. ISSUE 1: 'Slide to Send' Button not appearing for collectibles on Mainnet, Optimism, and ArbitrumSteps:
Actual result:The 'Slide to send' button does not appear when the collectible is opened. slide_tosend.mp4Expected result:The 'Slide to send' button should appear after the collectible is opened. Logs: |
Question 1:Should the 'send' button work and navigate to the sending process if it is tapped on the collectible detailed page? Currently, it doesn't. send_tap.mp4 |
77% of end-end tests have passedFailed tests (9)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (37)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
|
|
Hi @VolodLytvynenko! So I've been looking into the issue and checked that the collectibles that are not working are those of type ERC-1155. As per discussed with @alaibe, currently we only support ERC-721 collectibles, tested some ERC-721 and it worked fine for me. Please be sure that you are testing with ERC-721 collectibles, and let's create corresponding issues in status-go and status-mobile to support ERC-1155 collectibles. I'll update the PR description to explicitly specify that this PR adds support only for ERC-721 collectibles. At the meantime, improved the feedback to the user when routes are not found when sending a collectible. About question 1, we can work on that in a follow up issue as well. This PR just makes the transaction work in the basic happy path. |
briansztamfater
force-pushed
the
fix/reintroduce-collectibles-erc20
branch
from
c42f525
to
ccad9ef
Compare
briansztamfater
changed the title
OmarBasem
approved these changes
Feb 1, 2024
78% of end-end tests have passedFailed tests (2)Click to expandClass TestDeepLinksOneDevice:
Passed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Hi @briansztamfater Indeed, all collectibles I have are ERC-1155. Created a follow up for this. No additional issue from my side. PR can be merged. Thank you |
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
fix/reintroduce-collectibles-erc20
branch
from
ccad9ef
to
7bc6777
Compare
ulisesmac
pushed a commit
that referenced
this pull request
Feb 2, 2024
…8679) Signed-off-by: Brian Sztamfater <brian@status.im>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes ?
Summary
This PR reintroduces changes to send ERC721 and ERC20 tokens that were accidentally removed in a rebase. Linked issue should be fixed as well, but also we should test the send flow is working.
Review notes
No new features here, just rollback some changes
status: ready