New component: zcash_address crate #352

str4d · 2021-03-08T04:53:59Z

The idea is that zcash_address encapsulates parsing and serialization logic for Zcash address encodings, in such a way that downstream users:

get automatic parsing support for future Zcash address formats simply by upgrading the dependency;
get separate error messages for "invalid / unknown" vs "unsupported" addresses.
- From past experience, these error messages are often just directly displayed to the user, so this helps to inform the user about why the address they pasted is not working.

Part of #371.

codecov · 2021-03-08T05:14:41Z

Codecov Report

Merging #352 (1af7b01) into master (05bd98b) will decrease coverage by 0.29%.
The diff coverage is 36.42%.

❗ Current head 1af7b01 differs from pull request most recent head af02e11. Consider uploading reports for the commit af02e11 to get more accurate results

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
- Coverage   64.46%   64.17%   -0.30%     
==========================================
  Files          73       72       -1     
  Lines        7229     7212      -17     
==========================================
- Hits         4660     4628      -32     
- Misses       2569     2584      +15

Impacted Files	Coverage Δ
components/zcash_address/src/convert.rs	`0.00% <0.00%> (ø)`
components/zcash_address/src/lib.rs	`0.00% <0.00%> (ø)`
components/zcash_address/src/encoding.rs	`50.49% <50.49%> (ø)`
zcash_client_backend/src/address.rs	`63.15% <0.00%> (-15.79%)`	⬇️
zcash_client_backend/src/zip321.rs	`63.66% <0.00%> (-5.32%)`	⬇️
zcash_client_sqlite/src/lib.rs	`51.69% <0.00%> (-2.93%)`	⬇️
zcash_primitives/src/consensus.rs	`19.44% <0.00%> (-2.78%)`	⬇️
...sh_primitives/src/transaction/components/amount.rs	`43.75% <0.00%> (-2.09%)`	⬇️
zcash_history/src/node_data.rs	`64.81% <0.00%> (-1.54%)`	⬇️
zcash_history/src/tree.rs	`59.83% <0.00%> (-0.98%)`	⬇️
... and 58 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 6fab0c8...af02e11. Read the comment docs.

components/zcash_address/src/encoding.rs

components/zcash_address/README.md

daira · 2021-03-13T14:56:40Z

components/zcash_address/src/encoding.rs

+                    _ => {
+                        // Use some heuristics to try and guess whether this might be a Zcash
+                        // address from the future:
+                        // - Zcash HRPs always start with a 'z'.


It's quite possible that we may want to add transparent address formats that use bech32[m] encoding. I would suggest that we allow "t" followed by any single character as an HRP for transparent addresses.

daira

utACK.

Is it intended that this be able to parse address bundles/unified addresses? I think the API is already compatible with that.

str4d · 2021-03-27T04:31:34Z

Yes; if/when we decide to use Unified Addresses, the Orchard key type would be replaced by UA parsing and serialization.

This provides round-trip encoding for Zcash addresses.

This enables easy conversion of an encoded Zcash address to a target type, with automatic handling of Zcash address types that are not supported by the target.

This places parsing documentation front and centre, while also making it clear that `str::parse` is the anticipated main entry point.

Using two separate `impl ZcashAddress` blocks resulted in separate blocks in the documentation, which is unnecessary.

This commit removes the now-undefined Orchard encoding logic, and adds the general Bech32m encoding/decoding logic for Unified Addresses. The internal data format of Unified Addresses is not correct in this commit.

str4d · 2021-05-25T15:35:32Z

Rebased PR.
Removed draft Orchard address encoding (which no longer exists).
Added initial Unified Address encoding.
Removed the future-address heuristics from this PR, as we now have the Unified Address format that we plan to use going forward.

I'm not sure whether this is the ideal API now that it includes UA support; I'm going to do some more thinking about it. But this should be mergeable as-is.

Co-authored-by: Daira Hopwood <[email protected]>

nuttycom

I think that the point at which f4jumble is being applied in the encoding process is incorrect. Otherwise, this looks good.

nuttycom · 2021-05-25T17:50:28Z

components/zcash_address/src/convert.rs

+    fn from_sprout(net: Network, data: sprout::Data) -> Self {
+        ZcashAddress {
+            net: if let Network::Regtest = net {
+                Network::Test


Does sprout not exist on regtest or something? Why is this different from sapling? As a third-party user, I think the impls that are written like this are rather surprising; if I pass in Network::Regtest and get back Network::Test I'll be rather confused as to what's going on.

Addendum: I later found the comment where Network::Regtest was defined; I think that the correct thing to do is handle the network values separately at the use site, not to rewrite them in this location.

components/zcash_address/src/kind/unified.rs

These need to be applied to the entire UA encoding, not to the encoding of each individual receiver.

daira · 2021-05-25T20:27:22Z

components/zcash_address/LICENSE-MIT

@@ -0,0 +1,21 @@
+The MIT License (MIT)
+
+Copyright (c) 2021 Electric Coin Company


Electric Coin Company, or "Zcash developers"?

ECC (matches the license text in zcash_primitives/LICENSE-MIT which is where the F4Jumble impl was moved from, and also matches ECC having copyright assignment for the rest of the code I've written in this module).

OK, but it'll need to be changed as soon as we get any external contributions, unless we intend to require copyright assignment (which we don't do for any of our other MIT/Apache code).

We've been accepting external contributions for quite some time with the rest of the repo licensed in this way. If there's a change that needs making, it should be made uniformly.

We've been accepting external contributions for quite some time with the rest of the repo licensed in this way.

I think this is violating the external contributors' copyrights, strictly speaking.

If there's a change that needs making, it should be made uniformly.

Agreed. Filed #393.

nuttycom

utACK ff94f66

nuttycom · 2021-05-25T20:52:18Z

components/zcash_address/src/kind/unified.rs

+    use super::{Address, Receiver};
+
+    prop_compose! {
+        fn uniform43()(a in uniform11(0u8..), b in uniform32(0u8..)) -> [u8; 43] {


Oh, this is a nice technique for larger byte arrays, I should have thought of this before! I've used vec(any::<u8>(), size) before but then had to convert.

One downside to this is the separate halves shrink separately (though that's less of an issue here given that these addresses are composite types). In theory we can implement our own custom array sizes directly, but I couldn't figure out how to set up the UniformArrayStrategy type properly.

daira · 2021-06-02T09:16:23Z

components/zcash_address/src/convert.rs

+    fn from_sprout(net: Network, data: sprout::Data) -> Self;
+
+    fn from_sapling(net: Network, data: sapling::Data) -> Self;
+


New code is ideally supposed to be constructing only unified, P2SH, and P2PKH addresses, right?

True; I added these for maximum compatibility (so you could replace your current parsing and serialization stack with zcash_address), but I guess until the old addresses go away, devs can use their existing serialization code.

I don't object to including them, but let's document that they are not preferred.

components/zcash_address/src/encoding.rs

daira · 2021-06-02T23:19:04Z

components/zcash_address/src/encoding.rs

+                    unified::MAINNET => Network::Main,
+                    unified::TESTNET => Network::Test,
+                    unified::REGTEST => Network::Regtest,
+                    // We will not define new Bech32m address encodings.


I think it's possible that we will. For instance, we may find a way to reduce the overhead currently imposed by the 16 bytes of redundancy, or some other way to mitigate address replacement attacks that doesn't use F4Jumble. Or there could be some cryptographic flaw in that design that we need to fix later.

Mmk, I'll remove this comment. But since there's no way to predict how we might make that kind of change, I can't do what I was previously doing (returning ParseError::MaybeZcash).

I think that we should reserve another Bech32m HRP for such purposes (just one for each network should be sufficient).

components/zcash_address/src/encoding.rs

daira · 2021-06-03T09:15:43Z

components/zcash_address/src/kind/unified.rs

+            .chain(iter::repeat(0).take(PADDING_LEN))
+            .collect();
+
+        f4jumble::f4jumble(&encoded).unwrap()


I believe this could panic if the length of encoded were greater than 16448 bytes.

Currently there is no way to construct a unified::Address other than decoding one (which checks the length), so this cannot panic. When we add other constructors, we can similarly enforce correctness there.

daira

utACK modulo previous comments.

Co-authored-by: Daira Hopwood <[email protected]>

components/zcash_address/src/encoding.rs

Co-authored-by: Daira Hopwood <[email protected]>

daira reviewed Mar 8, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

daira reviewed Mar 8, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

r3ld3v added this to the Core Sprint 2021-08 milestone Mar 8, 2021

str4d force-pushed the zcash_address branch from fb0e78b to 8d7bdc5 Compare March 12, 2021 22:02

str4d marked this pull request as ready for review March 12, 2021 22:24

str4d modified the milestones: Core Sprint 2021-08, Core Sprint 2021-10 Mar 12, 2021

str4d force-pushed the zcash_address branch from 91b777d to 1af7b01 Compare March 12, 2021 22:33

daira reviewed Mar 13, 2021

View reviewed changes

components/zcash_address/README.md Outdated Show resolved Hide resolved

daira reviewed Mar 13, 2021

View reviewed changes

daira approved these changes Mar 13, 2021

View reviewed changes

nuttycom mentioned this pull request Mar 22, 2021

Generalize Sapling note encryption to allow reuse with Orchard notes. #358

Merged

r3ld3v modified the milestones: Core Sprint 2021-10, Core Sprint 2021-12 Mar 29, 2021

r3ld3v modified the milestones: Core Sprint 2021-12, Core Sprint 2021-14 Apr 12, 2021

str4d mentioned this pull request Apr 14, 2021

Implement parsing and serialization of Unified Addresses #371

Closed

str4d added 9 commits May 20, 2021 14:54

zcash_address: Initial empty library crate

c754363

zcash_address: Add address-encoding support

ec77175

This provides round-trip encoding for Zcash addresses.

zcash_address: ZcashAddress::convert() -> T: FromAddress

a366460

This enables easy conversion of an encoded Zcash address to a target type, with automatic handling of Zcash address types that are not supported by the target.

zcash_address: Extend MaybeZcash heuristics to 64-byte addresses

f7b1058

zcash_address: Add ZcashAddress::try_from_encoded method

c7fcee2

This places parsing documentation front and centre, while also making it clear that `str::parse` is the anticipated main entry point.

zcash_address: Move ZcashAddress::convert into root

b9f7049

Using two separate `impl ZcashAddress` blocks resulted in separate blocks in the documentation, which is unnecessary.

zcash_address: Document ZcashAddress::convert

ff07eea

zcash_address: impl {Clone, Copy, Eq, Hash} for ZcashAddress

ce8797e

zcash_address: Add convert::ToAddress helper trait

9f7398c

str4d force-pushed the zcash_address branch from 1af7b01 to 9f7398c Compare May 20, 2021 14:01

str4d added 3 commits May 20, 2021 22:50

zcash_address: Replace Orchard address encodings with Unified Addresses

ae2b8bf

This commit removes the now-undefined Orchard encoding logic, and adds the general Bech32m encoding/decoding logic for Unified Addresses. The internal data format of Unified Addresses is not correct in this commit.

Move F4Jumble implementation into zcash_address

6717cd8

zcash_address: Implement Unified Address encoding / decoding

e982d72

str4d added this to the Core Sprint 2021-20 milestone May 25, 2021

Update components/zcash_address/README.md

c2b5704

Co-authored-by: Daira Hopwood <[email protected]>

nuttycom requested changes May 25, 2021

View reviewed changes

str4d added 2 commits May 25, 2021 21:24

zcash_address: Add failing test showing unified::Address encoding bug

ff8695d

zcash_address: Fix padding and F4Jumble positions in Address::to_bytes

ff94f66

These need to be applied to the entire UA encoding, not to the encoding of each individual receiver.

daira reviewed May 25, 2021

View reviewed changes

nuttycom approved these changes May 25, 2021

View reviewed changes

daira reviewed Jun 2, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

daira reviewed Jun 2, 2021

View reviewed changes

daira reviewed Jun 3, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

daira reviewed Jun 3, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

daira reviewed Jun 3, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

daira reviewed Jun 3, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

daira reviewed Jun 3, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

daira reviewed Jun 3, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

daira reviewed Jun 3, 2021

View reviewed changes

daira approved these changes Jun 5, 2021

View reviewed changes

zcash_address: Use exhaustive matching in Display impl

f36034c

Co-authored-by: Daira Hopwood <[email protected]>

str4d commented Jun 6, 2021

View reviewed changes

components/zcash_address/src/encoding.rs Outdated Show resolved Hide resolved

str4d and others added 2 commits June 6, 2021 23:58

zcash_address: Minor cleanups

1590565

Co-authored-by: Daira Hopwood <[email protected]>

zcash_address: Fix clippy lint

af02e11

str4d merged commit 2f3e498 into zcash:master Jun 6, 2021

str4d deleted the zcash_address branch June 6, 2021 23:36

str4d mentioned this pull request Jul 12, 2021

Add Unified Address parser structural checks #416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New component: zcash_address crate #352

New component: zcash_address crate #352

str4d commented Mar 8, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading

daira Mar 13, 2021 •

edited

Loading

daira left a comment

str4d commented Mar 27, 2021

str4d commented May 25, 2021

nuttycom left a comment

nuttycom May 25, 2021

nuttycom May 25, 2021

daira May 25, 2021

str4d May 25, 2021

daira Jun 1, 2021

str4d Jun 2, 2021

daira Jun 3, 2021 •

edited

Loading

nuttycom left a comment

nuttycom May 25, 2021

str4d May 25, 2021

daira Jun 2, 2021

str4d Jun 2, 2021

daira Jun 3, 2021

daira Jun 2, 2021 •

edited

Loading

str4d Jun 2, 2021

daira Jun 3, 2021 •

edited

Loading

daira Jun 3, 2021

str4d Jun 6, 2021

daira left a comment

		@@ -0,0 +1,21 @@
		The MIT License (MIT)

		Copyright (c) 2021 Electric Coin Company

		fn from_sprout(net: Network, data: sprout::Data) -> Self;

		fn from_sapling(net: Network, data: sapling::Data) -> Self;

New component: zcash_address crate #352

New component: zcash_address crate #352

Conversation

str4d commented Mar 8, 2021 • edited Loading

codecov bot commented Mar 8, 2021 • edited Loading

Codecov Report

daira Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d commented Mar 27, 2021

str4d commented May 25, 2021

nuttycom left a 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

daira Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

nuttycom left a 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

daira Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

str4d commented Mar 8, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading

daira Mar 13, 2021 •

edited

Loading

daira Jun 3, 2021 •

edited

Loading

daira Jun 2, 2021 •

edited

Loading

daira Jun 3, 2021 •

edited

Loading