Zcash shielded transactions #2472
Conversation
krnak
force-pushed
the
zcash-orchard
branch
from
5fb1ebb to
1698f15
Compare
krnak
force-pushed
the
zcash-orchard
branch
2 times, most recently
from
080d13c to
f544513
Compare
There was a problem hiding this comment.
Partial review only.
I did not review the cryptography. Engineering side, PR is looking good, and we can address outstanding code quality issues. In particular, it will be necessary for us to take over so that we can make this play nicely with some upcoming PRs.
There was a problem hiding this comment.
This is a full review of the core code. I still haven't looked at tests, and I haven't looked in full at the trezorlib code.
"More tests" are missing, in particular tests for ZcashGetAddress and ZcashGetViewingKey
in a lot of places there are missing type annotations. I have noted some, I'll try to figure out how to make the typechecker report them.
Feel free to squash the fixups up to now before continuing -- or not, up to you.
Ideally please rebase this on top of the current Rust PR state, and set the target branch to the Rust PR, so that it's clearer which commits belong to the Python part.
|
rebased to actual Rust PR |
| if not (msg.t_address_n or msg.z_address_n): | ||
| raise wire.DataError("t-address or z-address path expected") | ||
|
|
||
| coin = by_name(msg.coin_name) |
There was a problem hiding this comment.
ah, one thing I didn't notice before: this code is not actually showing the path warnings.
both get_raw_transparent and get_raw_orchard should contain code like this:
if msg.show_display:
await paths.validate_path(ctx, keychain, address_n)
| async def get_viewing_key( | ||
| ctx: Context, msg: ZcashGetViewingKey, keychain: OrchardKeychain | ||
| ) -> ZcashViewingKey: | ||
| await require_confirm_export_viewing_key(ctx, msg) |
There was a problem hiding this comment.
please add await paths.validate_path(ctx, keychain, msg.z_address_n) before this line
| self.rng = None | ||
|
|
||
| async def process_inputs(self) -> None: | ||
| await self.check_orchard_inputs_count() |
There was a problem hiding this comment.
please add something like:
if not SCHEMA_ZIP32.match(self.params.address_n):
await helpers.confirm_foreign_path(self.params.address_n)
(see also BasicApprover.add_internal_input)
|
device tests finished |
Hannsek
added
the
blocked
|
(converted to draft during PR cleanup. the status indicates that this is not ready for review / merging, but still needs additional work from our side) |
Orchard shielded transactions for Trezor T.
Links
ZFND grant
Zcash documentation (pdf)
documentation
milestone M.1 report
milestone M.2 report
Todos
ZCASH_SHIELDEDflagPending issues and pull requests
Allow creating notes from serialized parts zcash/orchard#346Circuit::from_action_contextconstructor zcash/orchard#348ValueCommitTrapdoor::from_bytesconstructor zcash/orchard#349ValueCommitment::deriveconstructor zcash/orchard#351uninline-portablefeature zcash/pasta_curves#47hash_to_curveAPI zcash/pasta_curves#46Firmware team