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

Added transaction type tag field. #2182

Merged
merged 10 commits into from
Nov 21, 2023
Merged

Added transaction type tag field. #2182

merged 10 commits into from
Nov 21, 2023

Conversation

murisi
Copy link
Contributor

@murisi murisi commented Nov 15, 2023

Describe your changes

Added transaction type tag field in order to aid hardware wallets in decoding transactions. More specifically the following changes were made:

  • Added a tag: Option<String> field to the Code type that is used in the code and extra data sections - this is now where a transaction's tag can be placed. This field is also included in the section hashes.
  • Added tag validation to the protocol. More specifically, if a tag is present in the Code type, then the protocol will ensure that the hash corresponding to wasm/hash/<tag> equals the hash in the commitment.
  • Modified the test vector generation code to branch on tag information rather than hashes to simulate what the Ledger app would be doing.
  • Modified the transaction construction process to populate the tag field with the appropriate WASM path whenever possible (essentially whenever the WASM code hash was obtained with query_wasm_code_hash)
  • Removed the dummy proof-of-work field that was in the WrapperTx header now that the Ledger app has been modified.
  • Printed out more transaction details in the test-vectors for the InitValidator transaction, namely: email, description, Discord handle, and website.
  • Updated all the unit tests to account for the fact that the transaction builder API has slightly changed, i.e. to account for the fact some functions now receive an additional tag parameter.

The corresponding changes to the Ledger app can be found here: Zondax/ledger-namada#16 .

Indicate on which release or other PRs this topic is based on

Namada v0.26.0.

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@murisi murisi force-pushed the murisi/tx-type-tag2 branch 3 times, most recently from 7efe564 to 3aace0d Compare November 15, 2023 11:27
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

The changes mostly look fine. Some of the SDK code needs to be cleaned up, but we can do that separately. My main request here is that we make sure that all fixed strings are constants defined somewhere instead of strings copied around.

apps/src/lib/node/ledger/shell/mod.rs Outdated Show resolved Hide resolved
tv.output_expert
.push(format!("Code hash : {}", HEXLOWER.encode(&code_hash.0)));
})?;
tv.output_expert.push(format!(
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is super super messy and I don't understand what exactly it's trying to do, but most of the messiness seems to be leftover from before. What exactly is the aim of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is there to produce the JSON test vectors for Ledger integration. See an example output of this function (called with some Tx argument) below:

{
"blob":"1d0000006c6f63616c6e65742e6664633665356661643365356535326433662d300023000000323032332d31312d31365431343a33313a31322e3030383437393437392b30303a303029e3fd2d0a8c786d5318be88f0be06629152ac26628396e28350f7c5b81b1d58f09f9bf315fe3b244703f3695cafff63b67156f799dc5c0742d1612cdd4897be0101000000000000000000000000000000000000000000000000000000000000000032fdd4e57f56519541491312d4e9089032244eca0048998ffa0340c473b72dad3604abd76581e71e4a334d0708ef754a0adcec66d80300000000000000a861000000000000000400000002b3078bd88b010000007c7a739c83e943d4a56a0fd4e4c52a9edc0d66d9105324bcc909619857a6683b010c00000074785f626f6e642e7761736d00b3078bd88b0100004b00000000f2d1fbf5a690f8ab12cfa6166425bec4d7569bb400e9a435000000000000000000000000000000000000000000000000000000000100ba4c9645a23343896227110a902af84e7b4a4bb30301000000c7fec5279e22792a9cad6346f8933c1b2249043e1a03c835030d4e71dfbac3e00000ba4c9645a23343896227110a902af84e7b4a4bb301000000000087d6e5a4617cce4c93120504a5f5db8c9ce1af0416e260c3fbe9066df3f3fdb2abfda0cac21b97b3e89b3c29013db345bd22548e8baf2df4e682bb4e1a041f0f03040000005b693f86a6a8053b79effacd031e2367a1d35cc64988795768920b296501374229e3fd2d0a8c786d5318be88f0be06629152ac26628396e28350f7c5b81b1d58f09f9bf315fe3b244703f3695cafff63b67156f799dc5c0742d1612cdd4897bed4bfd3e247c0ef6e2ab23983a793412fd94a78d9a08efaa94a3d6a977e3c601c01010000000048998ffa0340c473b72dad3604abd76581e71e4a334d0708ef754a0adcec66d8010000000000cfcc82f327627fed72368dd168663db755478675d812365b9c8b92c36acaaebf1fe9a0494aaf9e675d4b4f041ffebc5234d9da012721b1bd5d1bbc819ed56f04",
"index":0,
"name":"Bond_0",
"output":[
    "0 | Type : Bond",
    "1 | Source [1/2] : tnam1qxaye9j95ge58ztzyugs4yp2lp88kjjtk",
    "1 | Source [2/2] : vrwawgc",
    "2 | Validator [1/2] : tnam1q8edr7l456g032cje7npvep9hmzdw45mk",
    "2 | Validator [2/2] : sktpy8f",
    "3 | Amount : NAM 900.0"
],
"output_expert":[
    "0 | Code hash [1/2] : 7c7a739c83e943d4a56a0fd4e4c52a9edc0d66",
    "0 | Code hash [2/2] : d9105324bcc909619857a6683b",
    "1 | Source [1/2] : tnam1qxaye9j95ge58ztzyugs4yp2lp88kjjtk",
    "1 | Source [2/2] : vrwawgc",
    "2 | Validator [1/2] : tnam1q8edr7l456g032cje7npvep9hmzdw45mk",
    "2 | Validator [2/2] : sktpy8f",
    "3 | Amount : NAM 900.0",
    "4 | Timestamp : 2023-11-16 14:31:12.008479479 UTC",
    "5 | Pubkey [1/2] : tpknam1qpyfnrl6qdqvguah9kknvp9t6ajcrec",
    "5 | Pubkey [2/2] : 7fge56pcgaa655zkua3nds48x83t",
    "6 | Epoch : 3",
    "7 | Gas limit : 0.025",
    "8 | Fees/gas unit : NAM 0.000001"
],
"valid":true
},

This JSON output format and its pagination is mostly determined by Zondax. The fact that the outputs are user-facing necessitates some amount of text formatting code. Also this code is structured to simulate how a hardware wallet might decode a transaction blob in order to ensure that the blob has sufficient information to decode, say, a MASP transfer from it. That being said, I'm responsible for essentially all of the messiness and will think of a way to fix it.

@murisi murisi marked this pull request as ready for review November 17, 2023 08:31
This was referenced Nov 17, 2023
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Changes look alright to me. Can you point me to where in the transaction execution code we check that the tag and hash correspond to each other correctly?

@murisi
Copy link
Contributor Author

murisi commented Nov 20, 2023

Changes look alright to me. Can you point me to where in the transaction execution code we check that the tag and hash correspond to each other correctly?

Hi. This is where we check that the transaction type tag and hash correspond in the transaction execution code:

// If the transaction code has a tag, ensure that the tag hash equals the
.

In the transactions that contain VP code hashes like InitAccount, InitValidator, and UpdateAccount, the tag and hash correspondence is checked here:

/// Validate a VP WASM code hash in a tx environment.
.

@tzemanovic tzemanovic merged commit 0ebfc46 into main Nov 21, 2023
12 of 14 checks passed
@tzemanovic tzemanovic deleted the murisi/tx-type-tag2 branch November 21, 2023 07:57
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.

3 participants