Sapling keys and addresses

[dconnolly]

I need test vectors! But otherwise ready for review.
Test vectors acquired and passing!

[daira]

ACK

[hdevalence]

Should this be used internally to a Display impl? Is there a reason that it shouldn't be just a Display impl? (e.g., is it called elsewhere in some context where Display can't be used?)

The function takes a network parameter. Should this be stored as part of the address structure itself, so that the address contains all the information required to render itself?

[dconnolly]

Unfortunately the actual structure of Sapling addresses is technically independent of the network (unlike Sprout), so when they are serialized/deserialized on the wire the network is not involved at all. So when we deserialize a Sapling z-addr, we know nothing about its network. It's only when they are pretty-printed for human consumption that the network is thrown in basically to help the consuming software look at the right data elsewhere, I think @str4d told me once that the params for prod/test networks in Sapling are the same so this is why???

https://zips.z.cash/protocol/protocol.pdf#saplingpaymentaddrencoding

[dconnolly]

And this holds for all the Sapling keys etc that define an encoding.

[hdevalence]

😢

Maybe it's possible to figure out some kind of workaround.... 🤔

[str4d]

Unfortunately the actual structure of Sapling addresses is technically independent of the network (unlike Sprout)

It's true for Sprout as well. It only happens to be the case that Sprout addresses are serialized with a "prefix" such that when Base58Check-encoding them, you get the nice human-readable prefix chars. Those prefix chars are an implementation detail, and the Sprout address itself is network-independent.

[dconnolly]

Actually, this only helps us when the address is bech32 encoded, but when it's not, it still doesn't include the network data.

[str4d]

In practice, we do associate the use of an address with a particular network, so even though the "raw encoding" doesn't include that data, it's reasonable for the struct to do so. The raw encoding for Sapling is only used publicly as the input to / output of Bech32 encoding (we also use it in zcashd to move PaymentAddress structs across the FFI, but that's an internal implementation detail).

[dconnolly]

OK so. We only encoded network for bech32. The actual raw encoding doesn't include it, for any Sapling types. For addresses, where we really only care about the bech32 encoding, we can just forego our normal Zcash(De)Serialize impls and just to 'to human readable'.

What about the other key types? The IncomingViewingKeys, FullViewingKeys, and SpendingKeys also have this issue.

[dconnolly]

We may want the 'human readable' equivalent of Zcash(De)Serialize instead for these.
None of those key types are inputs to transactions or messages.

[dconnolly]

OK! We're basically going to copy what we've done in Sprout types by including the network field and implementing Display for all of them as the human-readable component.

[hdevalence]

Didn't check against the spec yet, but looks good so far!

[dconnolly]

@hdevalence Is this considered good style/practice? Seems ok to me but just wondering if there's a better way I'm not aware of.

[hdevalence]

Hmm, I'm not sure. From is sort of magical, which is nice when magic is desired. Do we want magic here?

Also, another thing about From is that it works on owned values. So unless AuthorizingKey, NullifierDerivingKey are Copy, using this conversion might require cloning the keys unnecessarily, unless the From impl was impl<'a, 'b> From<(&'a AuthorizingKey, &'b NullifierDerivingKey)> ... which is a bit un-fun.

[hdevalence]

That said, I like From impls, in general..... I think this is sort of an aesthetic choice