Implement ZIP 302 memos #177
Conversation
|
Best reviewed commit-by-commit, because one of the commits is a move-only commit. |
str4d
changed the title
str4d
force-pushed
the
175-memo-enum
branch
2 times, most recently
from
1627c85
to
7d3b090
Compare
|
I rebased on master to fix merge conflicts. I also reordered the commits so the move happens before the refactor, cleaned up the history, and removed structured memo support (for now, while we decide how to handle them in ZIP 302). |
Codecov Report
@@ Coverage Diff @@
## master #177 +/- ##
==========================================
- Coverage 64.64% 64.38% -0.26%
==========================================
Files 69 70 +1
Lines 7079 7157 +78
==========================================
+ Hits 4576 4608 +32
- Misses 2503 2549 +46
Continue to review full report at Codecov.
|
str4d
force-pushed
the
175-memo-enum
branch
2 times, most recently
from
5af1c1d
to
25d5809
Compare
str4d
force-pushed
the
175-memo-enum
branch
3 times, most recently
from
7df2de5
to
e44e924
Compare
The MemoBytes struct is a minimal wrapper around the memo bytes, and only imposes the existence of null-padding for shorter memos. The only error case is attempting to construct a memo that is too long. MemoBytes is guaranteed to be round-trip encodable (modulo null padding). The Memo enum implements the additional memo rules defined in ZIP 302, interpreting the contents of a memo (for example, parsing it as text).
Memo fields have two ways to encode an empty memo: - 0xF6 followed by all-zeroes, encoding "there is no memo". - All-zeroes, encoding the empty UTF-8 string. In almost all cases you want the former, but users thinking about byte slices may expect MemoBytes::default() to result in the latter. To ensure clarity, we now require calling either MemoBytes::default() or MemoBytes::from_bytes(&[]) to be explicit. No such confusion exists for the Memo enum, because the two types are visibly separated as different enum cases, and Memo::Empty makes sense as the default.
|
After additional consideration, I really think that |
|
The problem is that the line is fuzzy here. For example, note encryption for non-shielded coinbase outputs is not "consensus critical" (and cannot be without the ZKPs checking the encryption inside the circuit, which we aren't going to do), but it is absolutely a core part of the protocol that everyone needs to agree on, or else users lose funds. I'm not against moving |
| match bytes.0[0] { | ||
| 0xF6 if bytes.0.iter().skip(1).all(|&b| b == 0) => Ok(Memo::Empty), | ||
| 0xFF => Ok(Memo::Arbitrary(Box::new(bytes.0[1..].try_into().unwrap()))), | ||
| b if b <= 0xF4 => str::from_utf8(bytes.as_slice()) |
There was a problem hiding this comment.
I just noticed that this doesn't yet implement the 0xF5 case specified in ZIP 302. That specification also needs some clarification related to how to parse the length bytes (see zcash/zips#105 (comment)).
There was a problem hiding this comment.
I intentionally removed 0xF5 handling from this PR last year, in an effort to make the PR mergeable, as it was the most contentious part.
We're removing those from the ZIP draft until they can be agreed upon.
Memois now an enum, which improves type safety.Closes #175. Part of zcash/zcash#1849.