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

[TON] Support TON wallet V5R1 external signed message #3983

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

10gic
Copy link
Contributor

@10gic 10gic commented Aug 13, 2024

Description

Support TON wallet V5R1.

There are three opcodes: signed_external, signed_internal, and extension_action. Currently, only signed_external is supported. It is sufficient for transferring native tokens, jettons, etc.

How to test

Run Rust, C++, iOS, Android tests

Types of changes

TON Breaking change:

  1. Move Proto::Transfer::wallet_version to Proto::SigningInput::wallet_version, so the upstream code will also need to be adjusted accordingly.

Checklist

  • Create pull request as draft initially, unless its complete.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • If there is a related Issue, mention it in the description.

If you're adding a new blockchain

  • I have read the guidelines for adding a new blockchain.

@satoshiotomakan
Copy link
Collaborator

Hi @10gic, thank you for the extensive work! I'll do a proper PR review later this week 👍

@10gic
Copy link
Contributor Author

10gic commented Aug 16, 2024

Hi @10gic, thank you for the extensive work! I'll do a proper PR review later this week 👍

It's great to see this. I'm really looking forward to your comments.

Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Hi @10gic, thanks again for the good quality PR! When I designed TonWallet structure, I didn't expect V5 will be so different. Please consider the suggestions to make it easier to add new versions in the future

rust/chains/tw_ton/src/message/out_list/mod.rs Outdated Show resolved Hide resolved
rust/chains/tw_ton/src/message/out_list/mod.rs Outdated Show resolved Hide resolved
rust/chains/tw_ton/src/message/out_list/out_action.rs Outdated Show resolved Hide resolved
rust/frameworks/tw_ton_sdk/src/error.rs Outdated Show resolved Hide resolved
rust/chains/tw_ton/src/signing_request/builder.rs Outdated Show resolved Hide resolved
rust/chains/tw_ton/src/transaction.rs Outdated Show resolved Hide resolved
rust/tw_any_coin/tests/chains/ton/ton_sign.rs Outdated Show resolved Hide resolved
rust/tw_any_coin/tests/chains/ton/ton_sign.rs Show resolved Hide resolved
@10gic
Copy link
Contributor Author

10gic commented Aug 20, 2024

Hi @10gic, thanks again for the good quality PR! When I designed TonWallet structure, I didn't expect V5 will be so different. Please consider the suggestions to make it easier to add new versions in the future

Thank you very much for your valuable comments. Except for this one (moving Proto::Transfer::wallet_version to Proto::SigningInput::wallet_version), which might involve a breaking change and hasn't been adjusted for now, I have accepted and made adjustments to all the others.

@10gic
Copy link
Contributor Author

10gic commented Aug 21, 2024

Hi @Vero7979. Thank you for recognizing my work. May I ask if you are from the trustwallet team?

@satoshiotomakan
Copy link
Collaborator

@10gic could you please resolve merge conflicts?

Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

One minor change please

rust/chains/tw_ton/src/signing_request/builder.rs Outdated Show resolved Hide resolved
@10gic
Copy link
Contributor Author

10gic commented Aug 21, 2024

@10gic could you please resolve merge conflicts?

Sure, I have rebased my branch.

Copy link
Collaborator

@satoshiotomakan satoshiotomakan 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!

@10gic
Copy link
Contributor Author

10gic commented Aug 21, 2024

Hi @Milerius @ar-g, I would greatly appreciate it if you could take the time to review this pull request.

@10gic
Copy link
Contributor Author

10gic commented Aug 21, 2024

The CI execution failed, and I am a bit curious why these issues didn't surface when I ran the script tools/build-and-test locally.

@satoshiotomakan
Copy link
Collaborator

@10gic
Copy link
Contributor Author

10gic commented Aug 21, 2024

@10gic, TWAnySignerTheOpenNetwork.SingMessageToTransferAndDeployWalletV5R1 C++ test has failed Please note there are a few other tests need to be changed accordingly to the wallet_version change in Proto. Android: https://github.com/trustwallet/wallet-core/blob/master/android/app/src/androidTest/java/com/trustwallet/core/app/blockchains/theopennetwork/TestTheOpenNetworkSigner.kt iOS: https://github.com/trustwallet/wallet-core/blob/master/swift/Tests/Blockchains/TheOpenNetworkTests.swift Wasm (JS): https://github.com/trustwallet/wallet-core/blob/master/wasm/tests/Blockchain/TheOpenNetwork.test.ts

Thank you so much. I will change it accordingly.

@satoshiotomakan
Copy link
Collaborator

Regarding tools/build-and-test script, it runs C++ tests only. Please re-run it, I guess something has been changed in the implementation, and the result Tx Cell should be updated.
For iOS, Android and Wasm tests, you can simply change the position of the wallet_version and push the changes, we can see the CI to pass. Though it's not difficult to configure Android, iOS, WASM environment, it still needs some time

@10gic
Copy link
Contributor Author

10gic commented Aug 21, 2024

Regarding tools/build-and-test script, it runs C++ tests only. Please re-run it, I guess something has been changed in the implementation, and the result Tx Cell should be updated. For iOS, Android and Wasm tests, you can simply change the position of the wallet_version and push the changes, we can see the CI to pass. Though it's not difficult to configure Android, iOS, WASM environment, it still needs some time

The failed Android/iOS/WASM test cases have been fixed, and tools/build-and-test was successful in my local environment. Could you help rerun the CI?

@10gic
Copy link
Contributor Author

10gic commented Aug 21, 2024

@10gic, TWAnySignerTheOpenNetwork.SingMessageToTransferAndDeployWalletV5R1 C++ test has failed Please note there are a few other tests need to be changed accordingly to the wallet_version change in Proto. Android: https://github.com/trustwallet/wallet-core/blob/master/android/app/src/androidTest/java/com/trustwallet/core/app/blockchains/theopennetwork/TestTheOpenNetworkSigner.kt iOS: https://github.com/trustwallet/wallet-core/blob/master/swift/Tests/Blockchains/TheOpenNetworkTests.swift Wasm (JS): https://github.com/trustwallet/wallet-core/blob/master/wasm/tests/Blockchain/TheOpenNetwork.test.ts

I have no idea why the case TWAnySignerTheOpenNetwork.SingMessageToTransferAndDeployWalletV5R1 would fail. I can run this case successfully in my local environment. Could you please help to rerun the CI so that I can check the log and gather more information for debugging?

@10gic
Copy link
Contributor Author

10gic commented Aug 21, 2024

@10gic, TWAnySignerTheOpenNetwork.SingMessageToTransferAndDeployWalletV5R1 C++ test has failed Please note there are a few other tests need to be changed accordingly to the wallet_version change in Proto. Android: https://github.com/trustwallet/wallet-core/blob/master/android/app/src/androidTest/java/com/trustwallet/core/app/blockchains/theopennetwork/TestTheOpenNetworkSigner.kt iOS: https://github.com/trustwallet/wallet-core/blob/master/swift/Tests/Blockchains/TheOpenNetworkTests.swift Wasm (JS): https://github.com/trustwallet/wallet-core/blob/master/wasm/tests/Blockchain/TheOpenNetwork.test.ts

I have no idea why the case TWAnySignerTheOpenNetwork.SingMessageToTransferAndDeployWalletV5R1 would fail. I can run this case successfully in my local environment. Could you please help to rerun the CI so that I can check the log and gather more information for debugging?

All checks have passed. It seems there is no issue with the test case.

@satoshiotomakan satoshiotomakan merged commit 75c73b1 into trustwallet:master Aug 22, 2024
12 checks passed
@10gic 10gic deleted the ton-wallet-v5 branch August 29, 2024 12:30
@10gic
Copy link
Contributor Author

10gic commented Aug 31, 2024

I finally figured out why the test case TWAnySignerTheOpenNetwork.SingMessageToTransferAndDeployWalletV5R1 occasionally fails; see issue 4004.

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.

2 participants