`zcash_primitives::legacy::keys::AccountPrivKey::{to_bytes, from_bytes}` does not use the `xprv` encoding about librustzcash HOT 3 CLOSED

Some of the missing information can be reconstructed from contextual information:

• ✅ depth is always 3, as AccountPrivKey is derived using the path m/44'/coin_type'/account'.
• ❌ We cannot determine the parent key fingerprint correctly without re-deriving the parent from the seed.
• ❓child number is account' (I believe including the hardening flag), which we "know" in the places we currently use the USK encoding (specifically for derived accounts in zcash_client_*), but don't necessarily have at the spot we are doing the decoding (in particular for imported UFVKs, support for which was recently added to zcash_client_*).

I also notice that AccountPubKey::{serialize, deserialize} is used in the UFVK encoding and has the same problem (for bip32 compatibility): it only encodes the chain code and pubkey. This uses the ordering chain code || pubkey and is therefore a suffix of the bytes used in the xpub encoding, rather than an entirely separate encoding. This encoding was intentional and is specified in ZIP 316; IIRC we decided that we didn't need any of this data and preferred the shorter UFVK encoding.

So in both cases we need to decide how to handle the unknown data. I think the "correct" way to handle this is to define AccountPrivKey and AccountPubKey as solely being wrappers around the chain code and pubkey / privkey, and then ensure there are no public APIs that expose the internal hdwallet (or soon-to-be bip32) types. Then when we migrate to bip32 we can just stick obviously-garbage data into the unknown fields.

The difference in encoding between AccountPrivKey (privkey || chaincode) and AccountPubKey (suffix of xpub) is annoying, and we may also want to fix this for AccountPrivKey.

from librustzcash.

I think it's fine to fix the USK encoding; the encoding methods are still under the unstable feature flag. Also, in order to stabilize this encoding, ZIP 316 would need to be updated to specify it, which hasn't yet been done.

The AccountPubKey encoding can stay as it is, I think; that choice was intentional.

from librustzcash.

I think it's fine to fix the USK encoding; the encoding methods are still under the unstable feature flag.

Agreed. The Android SDK already tells its users that they need to be prepared to regenerate the USK:
https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/159de09b2bf1cc1c752260c76d444c11dfabebc9/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/UnifiedSpendingKey.kt#L64-L74

The iOS SDK doesn't, but it also has a regeneration API like the Android SDK. And if we want this to be a joint we keep oiled, we need to exercise it.

from librustzcash.