Refactor transfer transaction creation and WASM to split shielded and transparent transfers

[cwgoes]

I audited the transfer transaction creation for any data leakage, and I didn't find any, but the way things are structured is very confusing, e.g. setting amount and token to sentinel amounts for MASP transactions. I think it would be clearer if we refactor transfers into four separate transactions:

• Transparent transfer
• Shielding
• Unshielding
• Shielded transfer

These should have separate WASM, separate functions, and separate CLI commands, and it should be clear what data is kept private and what data is sent to the ledger in each case (and this should be reflected by the WASM function's arguments).

[cwgoes]

Moving this to phase 1 so we can avoid transaction format changes.

[cwgoes]

@Fraccaman confirmed that this will not break the transaction format

[murisi]

@cwgoes Reviewing the changes in #3356 made me revisit this issue.

but the way things are structured is very confusing, e.g. setting amount and token to sentinel amounts for MASP transactions.

It seems to me that the introduction of vectorized transfers could nullify the need for a sentinel amount and sentinel token. This could be done by having a transfer whose sources and targets are empty vectors and yet still has a shielded_section_hash to effect shielded changes.

I think it would be clearer if we refactor transfers into four separate transactions:

* Transparent transfer

* Shielding

* Unshielding

* Shielded transfer

These should have separate WASM, separate functions, and separate CLI commands.

Furthermore, there may be some drawbacks in how we combine this splitting with the transfer vectorization changes:

• The Transfer wasms only support subset of total combinations of transfers. A transfer with transparent inputs, transparent outputs, and shielded (inputs or outputs) all there simultaneously would be a problem it seems. See: Vectorize transfers #3356 (comment)
• The MASP VP is not simplified by this splitting because it no longer branches depending on the kind of shield (i.e. transparent, shielding, shielded, or unshielding). Because the VP is only concerned with total inputs matching total outputs, it supports the simultaneous case identified in the above point (even as none of the transfer wasms support expressing it).
• There are now 4 different transfer formats that the hardware wallet needs to support for representing, parsing and printing. (Though incidentally the printing code was unified/reduced by assuming that the 4 formats are specializations of the original transfer format. However the vectorization changes may change hinder using this trick.)
• The transfer wasms are a little repetitive and are essentially straightforward specializations of the general case achieved by hardcoding the MASP address in certain places. A wasm for the general case has about the same amount of code as its specializations.

So overall the combination of this splitting and vectorization feels to me like an artificial limitation on the expressiveness of transfers with some potential maintenance overhead, especially in the hardware wallet. Increasing the expressiveness/coverage of the transfer formats in the future would also have to be coordinated with hardware wallet updates to support it.

it should be clear what data is kept private and what data is sent to the ledger in each case

A potentially simpler approach to achieve full expressivity + reduced maintenance overhead + and yet keep the above clarity in the context of vectorization might be to have one transfer format: a vectorized transparent transfer with an optional shielded_section_hash. (Or maybe two transfer types: one without shielded_section_hash and the other with shielded_section_hash.) One transfer wasm. But continue with separate CLI commands and separate builder functions as you suggested above.

[cwgoes]

Excellent points - this issue preceded transfer vectorization, and I hadn't considered the combination of the two at the time.

A potentially simpler approach to achieve full expressivity + reduced maintenance overhead + and yet keep the above clarity in the context of vectorization might be to have one transfer format: a vectorized transparent transfer with an optional shielded_section_hash. (Or maybe two transfer types: one without shielded_section_hash and the other with shielded_section_hash.) One transfer wasm. But continue with separate CLI commands and separate builder functions as you suggested above.

Yes, I think that would make sense. Do I correctly assume, however, that this would require a further refactor at the moment?

[murisi]

Excellent points - this issue preceded transfer vectorization, and I hadn't considered the combination of the two at the time.

A potentially simpler approach to achieve full expressivity + reduced maintenance overhead + and yet keep the above clarity in the context of vectorization might be to have one transfer format: a vectorized transparent transfer with an optional shielded_section_hash. (Or maybe two transfer types: one without shielded_section_hash and the other with shielded_section_hash.) One transfer wasm. But continue with separate CLI commands and separate builder functions as you suggested above.

Yes, I think that would make sense. Do I correctly assume, however, that this would require a further refactor at the moment?

Thanks. See #3446 for an attempt to achieve the above goals.