confidential asset types

[cbeck88]

This PR implements the transaction math part of confidential token ids.
MCIP (still draft) is here: mobilecoinfoundation/mcips#25

Still TODO:

• Add the proof of knowledge of opening
• Rebase on master
• Use the mob id constant and token id types in master
• Rename Amount -> MaskedAmount (that may come in a separate PR afterwards)
• More tests

---

More TODO

• Remove proof of knowledge of opening
• Undo breaking changes to protos
• Use the block version feature test in transaction builder and validation
• Make sample paykit support multiple asset balances
• Make mobilecoind track token ids
• Make slam and fog distro respect token ids
• rebase again
• Make it possible to bootstrap with token ids, and test in CD

[jcape]

As a general comment, the the AmountData {} struct seems really awkward to use, maybe just wrap it up entirely with Amount?

I dunno, I feel like this should be represented as:

pub struct Amount<T: Token> {
    value: u64,
    _type: PhantomData<T>,
}

impl<T: Token> Add<Amount<T>> for Amount<T> {
    /* ... */
}

And just use the token ID when serializing/deserializing. I realize our uses are more complex than that, but I worry that we're opening the door to a lot of type confusion bugs by not letting the compiler enforce this kind of thing wherever possible.

[cbeck88]

I dunno, I feel like this should be represented as:

One of the design requirements is, if we ship this functionality, it should be possible to stand up new token ids in prod without signing a new enclave.

If we make AmountData a template over the token id as you describe, then there is no way the enclave can create fee outputs for tokens that didn't exist when it was signed, because those types were not part of the build at that time. So this proposal is incompatible with the requirements. Let me know if you still have questions about this, or if you see some alternative.

[cbeck88]

And just use the token ID when serializing/deserializing. I realize our uses are more complex than that, but I worry that we're opening the door to a lot of type confusion bugs by not letting the compiler enforce this kind of thing wherever possible.

I think actually, the best mechanism we have for catching those bugs is the proof of opening, which is checked both by the signer after they construct the Tx, and by the consensus verifier. Even if there is a bug or a weak API in the TransactionBuilder where tokens of different types get mixed up, building the transaction will fail because they will be unable to create a valid proof of opening.

[eranrund]

On the topic of breaking changes, I am wondering what the plan is for getting this deployed. If we deploy this to the network before SDKs upgrade, then they will no longer be able to submit transactions.
If we want to update SDKs first, they will need to support constructing both old and new txs and decide based on the node they connect to whether to construct v1 or v2.

It would be nice if we could somehow make the Tx/TxPrefix change non-breaking so that for MOB transactions stuff just continues to work. Not sure how feasible that is with the signature change. But if we are making a breaking change I do think we should plan for either:

1. Having Consensus support accepting both old and new tx types for a time period (which might allow us to deploy this change prior to any SDK updates?)
2. Having the SDKs support both current and new Tx construction, and figure out how they decide which version to submit.

[cbeck88]

I don't know right now how we can make this not breaking. If we allow people to omit proof of opening, then they could mix token ids. So once we start having multiple token ids, the old transaction types are not valid.

We could maybe like, duplicate the transaction-core crate and have a transaction-core v2 crate. But it would be very painful.

I think it would be simpler to make the clients figure out based on the MRENCLAVE they get during attestation whether to use the v1 transaction builder or the v2 transaction builder

[cbeck88]

We could try to make the enclave depend on two different revisions of transaction core, that might avoid actually copy pasting all the code. But it's still going to be very complicated.

[eranrund]

That does mean that we need to figure out how to expose two versions of the TransactionBuilder for the SDKs.

[cbeck88]

@eranrund they might be able to just link an old and new version of libmobilecoin?

[cbeck88]

maybe there is a better way, maybe we can accept the old Txs as long as all the inputs and outputs are missing the masked_token_id, and then skip the proof of opening.

[cbeck88]

and then in a future revision, we require the masked token id and proof of opening?

that might be a better direction for this PR, gonna think on it some more

thanks for asking good questions you guys

[eranrund]

@eranrund they might be able to just link an old and new version of libmobilecoin?

I would be surprised, due to symbol name collision. Might be able to do some horrible linker tricks to get around it but I am not excited about that...

I also have no idea how the Java JNI stuff would behave.

[mfaulk]

Thanks, @garbageslam, this looks very good. I also appreciate the extra care to update the transaction crate's doc comments.

[cbeck88]

This has passed deploy (build network #46), I think we should merge it if there are no outstanding issues.

I didn't get the bootstrap with custom token ids to work, that seems to break deployment and I'm not sure why. But I think we can test that functionality end to end once minting is merged.

[eranrund]

LGTM, thank you for all your hard work on this.
I would prefer if this one gets more than two approvals before merging, preferably two more approvals from people intimately familiar with the codebase.

[eranrund]

If its not called can we remove it? Having incorrect code is worse than having a breaking build in my opinion.

[eranrund]

Nice

[mfaulk]

Thank you for all of your work on this, Chris!

[sugargoat]

This is awesome - thanks for the great work and prototype. Will make sure the MCIP is up to date with this as well!