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

fix(wallet) sending community tokens doesn't work #14126

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

stefandunca
Copy link
Contributor

@stefandunca stefandunca commented Mar 22, 2024

Closes #14074

The contract address is not resolved for the community tokens because of symbol usage which is not resolved by getTokenBySymbolByTokensKey.
This change switched to always using tokensKey to identify the token for all non-native transfers. Basically the transferToken always requires token which guarantees a valid token address.
Also remove token lookup redundant code.

@status-im-auto
Copy link
Member

status-im-auto commented Mar 22, 2024

Jenkins Builds

Click to see older builds (16)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 7e5c205 #1 2024-03-22 17:42:30 ~6 min tests/nim 📄log
✔️ 7e5c205 #1 2024-03-22 17:44:49 ~9 min macos/aarch64 🍎dmg
✔️ 7e5c205 #1 2024-03-22 17:47:10 ~11 min tests/ui 📄log
✔️ 7e5c205 #1 2024-03-22 17:48:14 ~12 min macos/x86_64 🍎dmg
✔️ 7e5c205 #1 2024-03-22 17:54:36 ~19 min linux/x86_64 📦tgz
✔️ 7e5c205 #1 2024-03-22 18:00:05 ~24 min windows/x86_64 💿exe
✔️ 721b72b #4 2024-03-25 15:29:43 ~5 min macos/aarch64 🍎dmg
✔️ 721b72b #4 2024-03-25 15:31:19 ~6 min tests/nim 📄log
✔️ 721b72b #4 2024-03-25 15:32:43 ~8 min macos/x86_64 🍎dmg
✔️ 721b72b #4 2024-03-25 15:36:26 ~11 min tests/ui 📄log
✔️ 721b72b #4 2024-03-25 15:41:26 ~16 min linux/x86_64 📦tgz
✔️ d9a0f46 #5 2024-03-27 10:23:52 ~4 min macos/aarch64 🍎dmg
✔️ d9a0f46 #5 2024-03-27 10:25:37 ~6 min tests/nim 📄log
✔️ d9a0f46 #5 2024-03-27 10:27:05 ~8 min macos/x86_64 🍎dmg
✔️ d9a0f46 #5 2024-03-27 10:30:15 ~11 min tests/ui 📄log
✔️ d9a0f46 #5 2024-03-27 10:35:49 ~16 min linux/x86_64 📦tgz
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ dd1db75 #6 2024-03-27 13:31:34 ~6 min tests/nim 📄log
✔️ dd1db75 #6 2024-03-27 13:32:13 ~6 min macos/aarch64 🍎dmg
✔️ dd1db75 #6 2024-03-27 13:34:07 ~8 min macos/x86_64 🍎dmg
✔️ dd1db75 #6 2024-03-27 13:37:09 ~11 min tests/ui 📄log
✔️ dd1db75 #6 2024-03-27 13:42:29 ~17 min linux/x86_64 📦tgz
✔️ 48c0507 #7 2024-03-28 12:50:21 ~4 min macos/aarch64 🍎dmg
✔️ 48c0507 #7 2024-03-28 12:52:43 ~7 min tests/nim 📄log
✔️ 48c0507 #7 2024-03-28 12:54:20 ~8 min macos/x86_64 🍎dmg
48c0507 #7 2024-03-28 12:57:27 ~11 min tests/ui 📄log
✔️ 48c0507 #7 2024-03-28 13:02:35 ~17 min linux/x86_64 📦tgz
✔️ 48c0507 #13 2024-04-02 10:38:43 ~32 min windows/x86_64 💿exe
✔️ 48c0507 #8 2024-04-03 06:03:57 ~10 min tests/ui 📄log

@stefandunca stefandunca force-pushed the fix_token_send-14074 branch from 7e5c205 to 2985068 Compare March 25, 2024 15:16
@stefandunca stefandunca marked this pull request as ready for review March 25, 2024 15:18
@stefandunca stefandunca force-pushed the fix_token_send-14074 branch 2 times, most recently from 37efaca to 721b72b Compare March 25, 2024 15:24
@stefandunca stefandunca force-pushed the fix_token_send-14074 branch 2 times, most recently from d9a0f46 to dd1db75 Compare March 27, 2024 13:25
The contract address is not resolved for the community tokens because
of symbol usage to identify assets. The symbol is not resolved by
`getTokenBySymbolByTokensKey`.
This fix changes from using symbol to always using assetKey to identify
the token for all non-native and non-collectibles transfers.

Closes #14074
@stefandunca stefandunca force-pushed the fix_token_send-14074 branch from dd1db75 to 48c0507 Compare March 28, 2024 12:45
Copy link
Contributor

@dlipicar dlipicar left a comment

Choose a reason for hiding this comment

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

There's still some conditions between the sendType and what we expect asset and assetKey to be that are not reflected in the code, but this is a big improvement vs what we had. That should perhaps be part of a bigger rework, split what's common and what's different for each case into smaller, separate functions.

Please test this with ETH, ERC20, ERC721 and ERC1155 transfers.

@stefandunca
Copy link
Contributor Author

stefandunca commented Apr 1, 2024

There's still some conditions between the sendType and what we expect asset and assetKey to be that are not reflected in the code, but this is a big improvement vs what we had. That should perhaps be part of a bigger rework, split what's common and what's different for each case into smaller, separate functions.

Please test this with ETH, ERC20, ERC721 and ERC1155 transfers.

I could validate that sending all types work after including fixes from here: status-im/status-go#5001

The PR is still blocked by the Windows CI that fails while fetching git submodules, waiting for Infra team to have the availability to fix this.

Copy link
Contributor

@Khushboo-dev-cpp Khushboo-dev-cpp left a comment

Choose a reason for hiding this comment

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

LGTM :)

@stefandunca stefandunca merged commit 65791a8 into master Apr 3, 2024
8 checks passed
@stefandunca stefandunca deleted the fix_token_send-14074 branch April 3, 2024 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not possible to send community asset with decimal points specified
4 participants