Connect swaps/swap claims #4035
Conversation
jessepinho
force-pushed
the
jessepinho/swap-swap-claim-connections-web-424
branch
2 times, most recently
from
3d730cd
to
bcf47d8
Compare
| // A map of base64-encoded nullfiers to the IDs of the transactions containing | ||
| // the outputs that they nullify. | ||
| map<string, txhash.v1.TransactionId> transaction_ids_by_nullifier = 7; | ||
| // A map of base64-encoded commitments to the IDs of the transactions | ||
| // containing those commitments. | ||
| map<string, txhash.v1.TransactionId> transaction_ids_by_commitment = 8; |
There was a problem hiding this comment.
Reviewers, please let me know what you think of this approach. It's the only way I could think of to easily allow views with links to other transactions to be generated from perspectives.
In the future, we'll also need to figure out a way to stuff output 1 and output 2 from the swap claim into the TransactionPerspective of a swap. But this work has already been taking me quite a while, so I wanted to at least get this merged first.
There was a problem hiding this comment.
Two comments:
-
I think the comment should read "transactions creating the commitments that they nullify" -- this is a minor nit but it's more technically correct since a swap commitment is a different kind of commitment than an output note
-
Why do we do base64 encoding of the nullifiers or commitments? This would force all of the consumers to add in special base64 parsing to interpret the perspective, right? Could we instead encode a list of
(Nullifier, TransactionId)or(Commitment, TransactionId)pairs?
There was a problem hiding this comment.
I think there are restrictions on what types Proto3 allows to be used in maps -- like they have to be scalar types (?) -- so this would mean defining two sub-messages, each modeling one of the pairs, and then having a repeated PairType (which is basically the way that a map will be encoded on the wire anyways). Then we'd change the domain type to have a BTreeMap<A, B> and deserialize the proto-gen-type's Vec<PairType> into BTreeMap<A, B> using .collect.
| asset_1_metadata: None, | ||
| asset_2_metadata: None, | ||
| batch_swap_output_data: None, |
There was a problem hiding this comment.
Don't we want this metadata too? We'd need it to render the assets (symbols, icons, etc) for a Swap without a corresponding SwapClaim right?
| let (output_1, output_2) = match bsod.map(|bsod| swap_plaintext.output_notes(bsod)) | ||
| { | ||
| Some((output_1, output_2)) => { | ||
| (Some(txp.view_note(output_1)), Some(txp.view_note(output_2))) | ||
| } | ||
| None => (None, None), | ||
| }; |
There was a problem hiding this comment.
The output notes of the swap are determined entirely by (1) the SwapPlaintext (committed to by the swap commitment and (2) the BatchSwapOutputData. This means that to get the output notes, we should supply a BSOD in the perspective, then recompute them.
Compared to trying to insert the output notes manually, this has two advantages:
- It correctly handles the case where the swap tx has been accepted on chain but the claim has not yet been submitted. In that case, even though we don't know the swap claim tx (it doesn't exist), we can still show the output notes.
- It allows rendering extra information (if we want), like the % share of the batch the user's swap was.
| asset_1_metadata: txp | ||
| .denoms | ||
| .get(&swap_plaintext.trading_pair.asset_1()) | ||
| .cloned(), | ||
| asset_2_metadata: txp | ||
| .denoms | ||
| .get(&swap_plaintext.trading_pair.asset_2()) | ||
| .cloned(), |
There was a problem hiding this comment.
Populating the asset_1_metadata and asset_2_metadata is important to be able to view the Swap action pre-submission, where there won't be any output notes.
| // Any relevant BatchSwapOutputData to the transaction. | ||
| // | ||
| // This can be used to fill in information about swap outputs. | ||
| repeated component.dex.v1.BatchSwapOutputData batch_swap_output_data = 60; |
There was a problem hiding this comment.
This needs to be repeated since there could be multiple different Swaps in the same tx.
hdevalence
force-pushed
the
jessepinho/swap-swap-claim-connections-web-424
branch
from
cb0cdbf
to
5ecb695
Compare
|
Rebased on main to resolve conflicts in the generated proto descriptors. |
|
Nice job! This was trickier than I expected. |
Despite the
SwapViewandSwapClaimViewtypes supporting fields that allow swaps and swap claims to reference each other, we haven't actually been populating those fields yet.This PR adds a couple keys to the
TransactionPerspectiveProtobuf type to make it possible to populate theclaim_txandswap_txfields inSwapViewandSwapClaimView, respectively. Hopefully, in the future, we can do the same with undelegates <-> undelegate claims.