Support calculating the TX hash of a transaction

10gic commented 2 months ago

Description

Support calculating the TX hash of a transaction. Fix issue https://github.com/trustwallet/wallet-core/issues/4001

How to test

Run Rust, C++, Kotlin, Swift tests

Types of changes

Checklist

[x] Create pull request as draft initially, unless its complete.
[x] Add tests to cover changes as needed.
[x] Update documentation as needed.
[x] 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 commented 2 months ago

Hi @10gic, I checked the PR. Do you plan to use this TWTransactionUtil module to calculate hash of just signed transaction using AnySigner.sign? I'd rather add SigningOutput.hash field in the signing result. That's because it's more clear to the users that they should look for this hash in the signing result than calling another module to get it.

satoshiotomakan commented 2 months ago

We already have hash field in some SigningOutputs. For example, in TheOpenNetwork::Proto::SigningOutput::hash. So please do not add a new module, but add the missing fields in proto files.

Please also note that there is one exception - Ethereum::Proto::SigningOutput::pre_hash is a hash to be signed, but not a hash of the signed transaction. So you can add one more field there Ethereum::Proto::SigningOutput::hash with a proper comment.

10gic commented 2 months ago

We already have hash field in some SigningOutputs. For example, in TheOpenNetwork::Proto::SigningOutput::hash. So please do not add a new module, but add the missing fields in proto files.

Please also note that there is one exception - Ethereum::Proto::SigningOutput::pre_hash is a hash to be signed, but not a hash of the signed transaction. So you can add one more field there Ethereum::Proto::SigningOutput::hash with a proper comment.

Hi @satoshiotomakan, as I mentioned in the issue https://github.com/trustwallet/wallet-core/issues/4001 :

There are cases where the TX is not constructed by the wallet core; for example, the transaction might be constructed by a DApp. The DApp only requests the wallet to perform the signing, constructs the transaction itself, and then asks the wallet to broadcast it.

The usage scenario is limited if it is only provided in the SigningOutput.

satoshiotomakan commented 2 months ago

@10gic most of the blockchain RPC nodes provide hash of the broadcasted transaction. Is it possible to leverage this in your case? I'm a little concerned that the number of such utilities in CoinEntry will grow

10gic commented 2 months ago

@10gic most of the blockchain RPC nodes provide hash of the broadcasted transaction. Is it possible to leverage this in your case? I'm a little concerned that the number of such utilities in CoinEntry will grow

Hi @satoshiotomakan, If we want to monitor the status of TX more thoroughly, relying on the RPC return result may be too late. This is because RPC return timeouts can occur, and in such cases, the TX might have already been submitted, but the client is unable to obtain the TX Hash.

10gic commented 2 months ago

Hi @satoshiotomakan, providing a generic function to calculate a transaction hash is a common feature. Many blockchain SDKs offer this capability. For example, okx js-wallet-sdk.

I was wondering if you might have plans to merge this pull request, or if there's anything else you'd like me to adjust. Thank you for your time!

10gic commented 2 months ago

Hi @satoshiotomakan, please let me know if there's anything else you would like me to adjust in the code. Thank you!

satoshiotomakan commented 2 months ago

Hi @10gic, could you please add unit tests in rust/tw_any_coin/tests/chains/{CHAIN}/{CHAIN}_transaction_util.rs for every blockchain you've implemented TransactionUtil module. Please also note that you can add a helper function like https://github.com/trustwallet/wallet-core/blob/c0cf03fe36c0b3116b7913bcb199cf3017f966b1/rust/tw_any_coin/src/test_utils/transaction_decode_utils.rs#L13

We need to keep code coverage high level

satoshiotomakan commented 2 months ago

Please also note that any change in CoinEntry trait requires symmetric changes in codegen-v2 tool: https://github.com/trustwallet/wallet-core/blob/406abe4b0649a74279b2e7b77a2b10d1bb61520c/codegen-v2/src/codegen/rust/templates/blockchain_crate/entry.rs#L25-L93

10gic commented 2 months ago

Please also note that any change in CoinEntry trait requires symmetric changes in codegen-v2 tool:

Thank you for the reminder, I have added it.

10gic commented 2 months ago

Hi @10gic, could you please add unit tests in rust/tw_any_coin/tests/chains/{CHAIN}/{CHAIN}_transaction_util.rs for every blockchain you've implemented TransactionUtil module. Please also note that you can add a helper function like

https://github.com/trustwallet/wallet-core/blob/c0cf03fe36c0b3116b7913bcb199cf3017f966b1/rust/tw_any_coin/src/test_utils/transaction_decode_utils.rs#L13

We need to keep code coverage high level

Sure, I've copied the C++ test cases into Rust.

10gic commented 2 months ago

Hi @satoshiotomakan, I've addressed the comments on the PR. Could you please let me know if there are any other comments or suggestions?

trustwallet / wallet-core