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

Add write_as_raw_encoding method to UnifiedAddress #711

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AloeareV
Copy link
Contributor

Adds a write_as_raw_encoding method to UnifiedAddress. Right now, Receivers can be read, turned into a zcash_address::unified::Address, and converted to a UnifiedAddress, but the reverse operation isn't possible.

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Base: 71.09% // Head: 70.95% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (e45977a) compared to base (580f87d).
Patch coverage: 13.63% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #711      +/-   ##
==========================================
- Coverage   71.09%   70.95%   -0.14%     
==========================================
  Files         115      115              
  Lines       11584    11604      +20     
==========================================
- Hits         8236     8234       -2     
- Misses       3348     3370      +22     
Impacted Files Coverage Δ
zcash_client_backend/src/address.rs 73.17% <13.63%> (-13.24%) ⬇️
zcash_client_backend/src/scan.rs 40.47% <0.00%> (-1.20%) ⬇️
zcash_primitives/src/sapling/redjubjub.rs 90.32% <0.00%> (-1.08%) ⬇️
zcash_history/src/entry.rs 27.08% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@AloeareV
Copy link
Contributor Author

I'm trying to clean up this code to remove that repetitive match statement, and it seems that hoops have been jumped through to keep typecode of a receiver private. Before continuing with this: Am I trying to make a change that's against the intent of librustzcash?

My goal is to write down a UA in as compact a format as possible, ideally while being network agnostic. Right now the only way to do this is to encode it for some network, decode it (for the same arbitrary network) as a zcash_address::unified::Address, call .items() on it, and then write down each individual reciever (by re-implementing a typecode match and unpacking the enum data, as typecode and data have been made private methods via the SealedItem trait I can't reference). I'm hoping to remove the intermediate step of encoding/decoding, but I feel like I'm working against the implementation at this point.

@nuttycom
Copy link
Contributor

What is the intent of exposing the raw encoding? It is intended that unified addresses are only ever serialized to their string encoding, not the raw byte encoding.

@daira
Copy link
Contributor

daira commented Nov 29, 2022

An example of wanting a binary encoding is for a ZIP 302 Reply-To field (because it is shorter than the string encoding). In this particular case, it's not necessary to encode the network because it is implied by the network of the transaction (since a shielded transaction always commits to the root of a commitment tree).

@str4d
Copy link
Contributor

str4d commented Nov 29, 2022

My goal is to write down a UA in as compact a format as possible, ideally while being network agnostic.

@AloeareV can you describe what your specific use case is?

Right now the only way to do this is to encode it for some network, decode it (for the same arbitrary network) as a zcash_address::unified::Address,

What type are you encoding from? Is it the UnifiedAddress type in zcash_client_backend or some other type of your own? This workflow shouldn't be necessary, as going directly from tightly-typed downstream types to zcash_address types should be possible via the existing Rust APIs. The UnifiedAddress type in zcash_client_backend might not expose this conversion, but if that's the case then we should enable it.

call .items() on it, and then write down each individual reciever (by re-implementing a typecode match and unpacking the enum data, as typecode and data have been made private methods via the SealedItem trait I can't reference).

We don't want to expose a raw binary encoding of UA receivers anywhere where it might end up being shown to users e.g. as hex (the whole reason for F4Jumble existing is to prevent various kinds of attacks on human-readable UA encodings). So I want to understand the use case here, to better inform how this kind of API should look (or whether it should exist at all).

@AloeareV
Copy link
Contributor Author

AloeareV commented Nov 30, 2022

Yeah, this is ZIP-302 arbitrary wallet-internal memo stuff. Right now when you send to a UA, only the selected receiver is actually encoded on-chain, meaning that when you restore a wallet from seed there's no way to recover the full UAs that were sent to. Zingolib is experimenting with using the memo field of the change output to solve this, and possibly some other things.. Right now it's just the UA(s) sent to, but the memo is only 511 bytes (plus the 0xFF byte), so I want to be able to store the sent-to UA as compactly as possible once we start to have more information to put there.

Now that I think about it more, I think I could get all the functionality I'm missing just by exposing the unknown receivers in the UnifiedAddress.

@AloeareV
Copy link
Contributor Author

AloeareV commented Nov 30, 2022

What type are you encoding from? Is it the UnifiedAddress type in zcash_client_backend or some other type of your own? This workflow shouldn't be necessary, as going directly from tightly-typed downstream types to zcash_address types should be possible via the existing Rust APIs. The UnifiedAddress type in zcash_client_backend might not expose this conversion, but if that's the case then we should enable it.

I'm encoding from a zcash_client_backend UnifiedAddress. As far as I've been able to tell, it's possible to convert a zcash_address::unified::Address to a zcash_address::ZcashAddress, or directly to a UnifiedAddress, but not the reverse. UnifiedAddress has a private to_address method that converts to a ZcashAddress, but it's not possible to go from a ZcashAddress to anything other than a string, or a crate-local type that I implement TryFromAddress on.

@nuttycom
Copy link
Contributor

After discussing this further, the correct way forward here is:

  • Amend ZIP 316 to explicitly specify a binary encoding for unified addresses, and state that this encoding MUST NOT be displayed to the user without being properly encoded (with f4jumble/Bech32m).
  • Refactor the current encoding function for unified addresses to expose this raw encoding function separately, instead of reimplementing it here. Clearly document that the newly exposed function may not be exposed to the user directly.
  • Additionally, propose & implement a ZIP 302 memo specification that standardizes how this information should be encoded in memos.

@nuttycom
Copy link
Contributor

@daira and I are working on specifying the binary encoding as an update to ZIP 316 this afternoon.

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.

4 participants