Add Unified Address parser structural checks #416
Conversation
Codecov Report
@@ Coverage Diff @@
## master #416 +/- ##
==========================================
- Coverage 61.45% 61.15% -0.30%
==========================================
Files 88 88
Lines 8465 8568 +103
==========================================
+ Hits 5202 5240 +38
- Misses 3263 3328 +65
Continue to review full report at Codecov.
|
These expose the receivers in sorted order, and in parsed order.
This enforces the same structural validity checks as at parsing time.
| }, | ||
| ] | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add a test for this case:
Senders MUST reject Unified Addresses/Viewing Keys in which any constituent address does not meet the validation requirements of its Receiver Encoding, as specified in the Zcash Protocol Specification.
An example is a UA containing a Sapling address with a non-canonical encoding (according to ZIP 216) of pkd.
There was a problem hiding this comment.
Also:
There MUST NOT be additional bytes at the end of the raw encoding that cannot be interpreted as specified above.
Examples would be:
- a UA that ends in a Receiver Encoding with a length field that exceeds the remaining number of bytes;
- a UA where the last Receiver Encoding is truncated after only the Typecode byte.
There was a problem hiding this comment.
Add a test for this case:
Senders MUST reject Unified Addresses/Viewing Keys in which any constituent address does not meet the validation requirements of its Receiver Encoding, as specified in the Zcash Protocol Specification.
An example is a UA containing a Sapling address with a non-canonical encoding (according to ZIP 216) of pkd.
zcash_address does not currently cover this case, as it only represents receivers using their raw encodings. It would be up to the downstream user of the crate to map these into their own types, and eventually perform those checks. In the case of zcashd the earliest we can check this is in the FFI right after this, but even if this were omitted we know it would eventually be rejected by the transaction builder.
Alternatively we could make zcash_address depend on all of the underlying crates (i.e. zcash_primitives, orchard) to parse them, and require the downstream user to have those dependencies in addition to their own.
| match self { | ||
| Typecode::P2pkh | Typecode::P2sh => false, | ||
| // Assume that unknown typecodes are shielded, because they might be. | ||
| _ => true, |
There was a problem hiding this comment.
It'd be better to explicitly list the remaining typecodes here, just for future-proofness. This reminds me, how will TZE interact with the addressing scheme?
There was a problem hiding this comment.
Note that in zcash/zips#536, I clarified the requirement that uses this as follows:
- A Unified Address or Unified Viewing Key MUST NOT contain only
transparent P2SH or P2PKH addresses (Typecodes :math:\mathtt{0x00}
and :math:\mathtt{0x01}). The rationale is that the existing
P2SH and P2PKH transparent-only address formats suffice for this
purpose and are already supported by the existing ecosystem.
So it is explicitly only P2SH and P2PKH addresses that are treated as transparent here (which fits with the rationale).
There was a problem hiding this comment.
@nuttycom we cannot list the remaining typecodes here, because they include Typecode::Unknown which is explicitly unbounded. As for TZE: we would need to decide at the time whether the property we want is "UA must contain a shielded component" (which would preclude TZE-only addrs) or "UA must not contain only a t-address-equivalent component" (which would enable the creation of non-shielded UAs). I suspect the latter is what we will want, treating UAs as the adapter mechanism for plugging in new receivers rather than needing to define a new address encoding every time.
There was a problem hiding this comment.
The presence of Typecode::Unknown makes me tempted to say that this function should actually return an Option<Bool> because otherwise callers can't use it in isolation of other checks against the typecode. This is probably fine; it just means that this won't be flagged by the compiler as a call site that needs inspection if the set of receivers changes. I'm just lazy, I want the compiler to do that work for me, rather than having to remember. :)
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Receiver ordering is now explicitly defined by Typecode, and Receiver is now a public type.
Per an NCC finding, these were accidentally omitted from #352.