Support calculating the TX hash of a transaction #4008

10gic · 2024-08-31T18:15:55Z

Description

Support calculating the TX hash of a transaction. Fix issue #4001

How to test

Run Rust, C++, Kotlin, Swift tests

Types of changes

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 · 2024-09-02T07:53:45Z

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 · 2024-09-02T07:58:12Z

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 · 2024-09-02T08:29:03Z

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 #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 · 2024-09-02T09:40:59Z

@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 · 2024-09-02T10:00:43Z

@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 · 2024-09-02T13:20:28Z

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!

include/TrustWalletCore/TWTransactionUtil.h

rust/chains/tw_solana/src/modules/transaction_util.rs

rust/frameworks/tw_utxo/src/modules/transaction_util.rs

satoshiotomakan · 2024-09-02T13:49:27Z

rust/tw_cosmos_sdk/src/modules/transaction_util.rs

+    fn calc_tx_hash_impl(_coin: &dyn CoinContext, encoded_tx: &str) -> SigningResult<String> {
+        let tx = base64::decode(encoded_tx, STANDARD)?;
+
+        let tx_hash = sha256(&tx);


tw_cosmos_sdk is used by many Cosmos-based chains. There is Greenfield that uses keccak256 hash instead of sha256. The type of hash is provided in Cosmos::Proto::SigningInput::tx_hasher.
Most likely we should allow an external user to specify some parameters for the transaction to be hashed correctly.
I'll think about how to better implement it. Brb in one/two days once I have something in mind

We can choose not to regard Greenfield as a Cosmos chain.

wallet-core/registry.json

Line 4184 in af67cae

"blockchain": "Greenfield",

And implement a GreenfieldTransactionUtil for Greenfield

I think it's fine to use CosmosContext::default_hasher(), so please refactor CosmosTransactionUtil to have a generic type parameter like:

struct CosmosTransactionUtil<Context: CosmosContext> { todo };

Thank you for your advice. I didn't realize that we already have the StandardCosmosContext. I've adjusted the code according to your suggestion.

I was planning to make calcTxHash support Greenfield as well, but I discovered that there might be some changes in Greenfield since the wallet core started supporting it. I noticed this because the links in the test cases are now returning 404 errors.

wallet-core/tests/chains/Greenfield/TransactionCompilerTests.cpp

Line 23 in f703596

// Successfully broadcasted https://greenfieldscan.com/tx/0x9f895cf2dd64fb1f428cefcf2a6585a813c3540fc9fe1ef42db1da2cb1df55ab

wallet-core/tests/chains/Greenfield/TWAnySignerTests.cpp

Line 15 in f703596

// Successfully broadcasted: https://greenfieldscan.com/tx/ED8508F3C174C4430B8EE718A6D6F0B02A8C516357BE72B1336CF74356529D19

Therefore, I won't be supporting it in this pull request.

Could you please review the code for me again?

satoshiotomakan · 2024-09-02T13:50:44Z

src/interface/TWTranscationUtil.cpp

+
+    const Rust::TWStringWrapper outputDataPtr = Rust::tw_transaction_util_calc_tx_hash(static_cast<uint32_t>(coinType), encodedTxStr.get());
+
+    return TWStringCreateWithUTF8Bytes(outputDataPtr.toStringOrDefault().c_str());


Better to return nullptr in case of an error

I think it's not bad to return an empty string in calcTxHash in case of an error.

It's a unified behaviour we try to follow in WalletCore. We already have some FFI functions that return an empty string or data in case of errors, but they're exceptions we plan to refactor.
For example, if a function returns TWString* _Nullable pointer, the wrapper function in Swift we generate will return String? that can be checked by try keyword

Thank you for providing the background. I will adjust the code accordingly.

10gic · 2024-09-04T05:38:37Z

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

satoshiotomakan · 2024-09-05T17:18:08Z

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

wallet-core/rust/tw_any_coin/src/test_utils/transaction_decode_utils.rs

Line 13 in c0cf03f

pub struct TransactionDecoderHelper<'a, Output: MessageRead<'a>> {

We need to keep code coverage high level

satoshiotomakan · 2024-09-05T17:25:56Z

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

wallet-core/codegen-v2/src/codegen/rust/templates/blockchain_crate/entry.rs

Lines 25 to 93 in 406abe4

    
           impl CoinEntry for {BLOCKCHAIN}Entry { 
        
               type AddressPrefix = NoPrefix; 
        
               type Address = {BLOCKCHAIN}Address; 
        
               type SigningInput<'a> = Proto::SigningInput<'a>; 
        
               type SigningOutput = Proto::SigningOutput<'static>; 
        
               type PreSigningOutput = CompilerProto::PreSigningOutput<'static>; 
        
               // Optional modules: 
        
               type JsonSigner = NoJsonSigner; 
        
               type PlanBuilder = NoPlanBuilder; 
        
               type MessageSigner = NoMessageSigner; 
        
               type WalletConnector = NoWalletConnector; 
        
               type TransactionDecoder = NoTransactionDecoder; 
        
               #[inline] 
        
               fn parse_address( 
        
                   &self, 
        
                   _coin: &dyn CoinContext, 
        
                   _address: &str, 
        
                   _prefix: Option<Self::AddressPrefix>, 
        
               ) -> AddressResult<Self::Address> { 
        
                   todo!() 
        
               } 
        
               #[inline] 
        
               fn parse_address_unchecked( 
        
                   &self, 
        
                   _coin: &dyn CoinContext, 
        
                   address: &str, 
        
               ) -> AddressResult<Self::Address> { 
        
                   {BLOCKCHAIN}Address::from_str(address) 
        
               } 
        
               #[inline] 
        
               fn derive_address( 
        
                   &self, 
        
                   _coin: &dyn CoinContext, 
        
                   _public_key: PublicKey, 
        
                   _derivation: Derivation, 
        
                   _prefix: Option<Self::AddressPrefix>, 
        
               ) -> AddressResult<Self::Address> { 
        
                   todo!() 
        
               } 
        
               #[inline] 
        
               fn sign(&self, coin: &dyn CoinContext, input: Self::SigningInput<'_>) -> Self::SigningOutput { 
        
                   {BLOCKCHAIN}Signer::sign(coin, input) 
        
               } 
        
               #[inline] 
        
               fn preimage_hashes( 
        
                   &self, 
        
                   coin: &dyn CoinContext, 
        
                   input: Self::SigningInput<'_>, 
        
               ) -> Self::PreSigningOutput { 
        
                   {BLOCKCHAIN}Compiler::preimage_hashes(coin, input) 
        
               } 
        
               #[inline] 
        
               fn compile( 
        
                   &self, 
        
                   coin: &dyn CoinContext, 
        
                   input: Self::SigningInput<'_>, 
        
                   signatures: Vec<SignatureBytes>, 
        
                   public_keys: Vec<PublicKeyBytes>, 
        
               ) -> Self::SigningOutput { 
        
                   {BLOCKCHAIN}Compiler::compile(coin, input, signatures, public_keys) 
        
               } 
        
           }

10gic · 2024-09-05T18:44:03Z

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 · 2024-09-05T18:45:46Z

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

wallet-core/rust/tw_any_coin/src/test_utils/transaction_decode_utils.rs

Line 13 in c0cf03f

pub struct TransactionDecoderHelper<'a, Output: MessageRead<'a>> {

We need to keep code coverage high level

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

10gic · 2024-09-06T10:54:24Z

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

satoshiotomakan

LGTM, thank you for the quick fixes!

10gic requested review from Milerius, satoshiotomakan and ar-g as code owners August 31, 2024 18:15

satoshiotomakan requested changes Sep 2, 2024

View reviewed changes

10gic force-pushed the support-calculating-tx-hash branch from 6dde9d8 to 1c96dd4 Compare September 2, 2024 15:58

10gic added 9 commits September 6, 2024 02:47

Support calculating the TX hash of a transaction

4605b96

Implement the calc_tx_hash for TON/Aptos/Sui

f2765b2

Adjust the code according to the comments

d8c8540

Refactor the calcTxHash for Solana

a1097d4

Format rust code using cargo fmt

209fec4

Change UtxoTransactionUtil to BitcoinTransactionUtil

f6b1e36

Add some comments

7c9fdb5

Adjust the code according to the comments

c46d66b

Add more test cases

0a96b87

10gic force-pushed the support-calculating-tx-hash branch from e092104 to 0a96b87 Compare September 5, 2024 18:47

satoshiotomakan previously approved these changes Sep 9, 2024

View reviewed changes

Fix issue found by cargo clippy

4967ad2

10gic dismissed satoshiotomakan’s stale review via 4967ad2 September 9, 2024 12:48

satoshiotomakan merged commit d88a263 into trustwallet:master Sep 9, 2024
12 checks passed

10gic deleted the support-calculating-tx-hash branch September 24, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support calculating the TX hash of a transaction #4008

Support calculating the TX hash of a transaction #4008

10gic commented Aug 31, 2024

satoshiotomakan commented Sep 2, 2024

satoshiotomakan commented Sep 2, 2024

10gic commented Sep 2, 2024

satoshiotomakan commented Sep 2, 2024

10gic commented Sep 2, 2024

10gic commented Sep 2, 2024

satoshiotomakan Sep 2, 2024

10gic Sep 2, 2024

satoshiotomakan Sep 4, 2024

10gic Sep 4, 2024

satoshiotomakan Sep 2, 2024

10gic Sep 2, 2024

satoshiotomakan Sep 4, 2024

10gic Sep 4, 2024

10gic commented Sep 4, 2024

satoshiotomakan commented Sep 5, 2024

satoshiotomakan commented Sep 5, 2024

10gic commented Sep 5, 2024

10gic commented Sep 5, 2024

10gic commented Sep 6, 2024

satoshiotomakan left a comment


		const Rust::TWStringWrapper outputDataPtr = Rust::tw_transaction_util_calc_tx_hash(static_cast<uint32_t>(coinType), encodedTxStr.get());

		return TWStringCreateWithUTF8Bytes(outputDataPtr.toStringOrDefault().c_str());

Support calculating the TX hash of a transaction #4008

Support calculating the TX hash of a transaction #4008

Conversation

10gic commented Aug 31, 2024

Description

How to test

Types of changes

Checklist

satoshiotomakan commented Sep 2, 2024

satoshiotomakan commented Sep 2, 2024

10gic commented Sep 2, 2024

satoshiotomakan commented Sep 2, 2024

10gic commented Sep 2, 2024

10gic commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

10gic commented Sep 4, 2024

satoshiotomakan commented Sep 5, 2024

satoshiotomakan commented Sep 5, 2024

10gic commented Sep 5, 2024

10gic commented Sep 5, 2024

10gic commented Sep 6, 2024

satoshiotomakan left a comment

Choose a reason for hiding this comment