Skip to content

Add transaction-builder suport for TZE-bearing transactions.#12

Merged
str4d merged 14 commits into
str4d:zip-tzesfrom
nuttycom:zip_tzes/txn_builder
Jun 4, 2020
Merged

Add transaction-builder suport for TZE-bearing transactions.#12
str4d merged 14 commits into
str4d:zip-tzesfrom
nuttycom:zip_tzes/txn_builder

Conversation

@nuttycom
Copy link
Copy Markdown

@nuttycom nuttycom commented May 19, 2020

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 19, 2020

Codecov Report

Merging #12 into zip-tzes will decrease coverage by 30.24%.
The diff coverage is 10.73%.

Impacted file tree graph

@@              Coverage Diff              @@
##           zip-tzes      #12       +/-   ##
=============================================
- Coverage     64.62%   34.38%   -30.25%     
=============================================
  Files           109       97       -12     
  Lines         15428    11663     -3765     
=============================================
- Hits           9970     4010     -5960     
- Misses         5458     7653     +2195     
Impacted Files Coverage Δ
zcash_extensions/src/consensus/transparent.rs 0.00% <0.00%> (ø)
zcash_extensions/src/transparent/demo.rs 0.00% <0.00%> (ø)
zcash_primitives/src/consensus.rs 11.32% <0.00%> (-0.68%) ⬇️
zcash_primitives/src/extensions/transparent.rs 0.00% <0.00%> (ø)
zcash_primitives/src/transaction/components.rs 28.32% <0.00%> (-5.76%) ⬇️
zcash_primitives/src/transaction/mod.rs 41.00% <ø> (-1.50%) ⬇️
zcash_primitives/src/transaction/builder.rs 43.69% <8.62%> (-7.03%) ⬇️
zcash_primitives/src/transaction/sighash.rs 79.74% <54.54%> (-9.96%) ⬇️
zcash_primitives/src/transaction/tests.rs 100.00% <100.00%> (ø)
bellman/src/groth16/generator.rs 0.00% <0.00%> (-90.37%) ⬇️
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbe713a...70e66c5. Read the comment docs.

Copy link
Copy Markdown
Owner

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Concept ACK.

Comment thread zcash_extensions/src/transparent/demo.rs Outdated
Comment thread zcash_extensions/src/transparent/demo.rs Outdated
Comment thread zcash_extensions/src/transparent/demo.rs
Comment thread zcash_extensions/src/transparent/demo.rs Outdated
Comment thread zcash_extensions/src/transparent/demo.rs
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
consensus_branch_id,
SIGHASH_ALL,
Some((i, &info.coin.script_pubkey, info.coin.value)),
// tze equivalent is ???
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think the approach we will end up taking is to serialize the TZE data in place of the transparent data, so this would become Option<InputData> with:

enum InputData {
    Transparent {
        index: usize,
        script_pubkey: Script,
        value: Amount,
    },
    Tze {
        index: usize,
        precondition: Vec<u8>,
        value: Amount,
    },
}

and we would domain-separate the transparent and TZE data types in the signature hash.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is the one piece that I think we should perhaps pair on; does this require a change to the ZIP?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yes, the ZIP will need to be updated to describe how it changes ZIP 243.

Comment thread zcash_primitives/src/extensions/transparent.rs
Comment thread zcash_primitives/src/extensions/transparent.rs Outdated
Comment thread zcash_primitives/src/extensions/transparent.rs Outdated
Comment thread zcash_primitives/src/consensus.rs Outdated
Comment thread zcash_primitives/src/consensus.rs Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Canopy has a consensus branch ID now, but let's leave these as placeholders so that anything attached to the Canopy NU in this branch is explicitly domain-separated from anything that might end up on mainnet.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

There has to be a nicer way to handle this, but we can figure it out in a future refactor of the Rust crates.

Comment thread zcash_primitives/src/transaction/mod.rs Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
@nuttycom nuttycom force-pushed the zip_tzes/txn_builder branch from 6d18217 to 3b44c5b Compare June 4, 2020 02:44
@nuttycom nuttycom force-pushed the zip_tzes/txn_builder branch from 3b44c5b to 4be71ef Compare June 4, 2020 02:46
@str4d str4d merged commit 1790571 into str4d:zip-tzes Jun 4, 2020
@nuttycom nuttycom deleted the zip_tzes/txn_builder branch June 4, 2020 19:51
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