-
Notifications
You must be signed in to change notification settings - Fork 1k
Handle IBC deposits/withdraws to/from payment addresses #4939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🧪 CI InsightsHere's what we observed from your CI run for ed70e61. 🟢 All jobs passed!But CI Insights is watching 👀 |
471aea1 to
ab45ed0
Compare
ab45ed0 to
2f12d8b
Compare
The sus fee can be obtained from the inner IBC args
4dde27b to
12e9a55
Compare
|
oops, I broke something between the last two f-pushes |
| } | ||
|
|
||
| /// Source of the MASP data we are validating. | ||
| enum MaspSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this. The code seems okay to me. Since it's been a while since I've looked at this code, I just have one or two basic questions: This PR allows IBC deposits into payment addresses (without generating MASP transactions). Is the main benefit of doing this avoiding having to generate zero-knowledge proofs, or is it something else? Is the main change of this PR that transaction notes are submitted in a vector instead of an actual Transaction object? Or is the change more fundamental? I think I have more questions/comments, but just want to make sure that I am understanding things right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the main benefit of doing this avoiding having to generate zero-knowledge proofs, or is it something else?
The immediate benefits I can think of are a few:
- Avoiding generating zero-knowledge proofs when depositing via IBC
i. This translates to less time to process the tx - Forgoing the need to include the generated MASP transaction in the memo field of the IBC transaction
i. This translates to cheaper txs on the counterparty chain, and cheaper IBC relaying fees - Refunding failed IBC unshielding transactions back to a payment address
i. Better UX, no need to manually shield refunds
Is the main change of this PR that transaction notes are submitted in a vector instead of an actual Transaction object?
In this PR, notes are generated by the protocol, the user doesn't submit any MASP data other than a payment address. This is possible due to the fact the submitted IBC ICS-20 packet data already has the necessary information to build a plaintext MASP note. Some of the other changes in the PR are required to figure out if an asset is part of the rewards program, emitting events for clients to pick up the notes minted by the protocol while syncing their local MASP state, etc.
12e9a55 to
12c5530
Compare
12c5530 to
ed70e61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying the context of this code. I see nothing obviously wrong or improvable with the changes to the MASP VP or the transfer context. I just wonder if we could utilize existing code and execution paths more in order to minimize risk. For instance, the transfer context has code to update the note commitment tree and the undated balances though apply_shielded_transfer does something similar. In the MASP VP, the new logic skips the anchor check execution paths when the ProtocolIbcShielding condition is met.
I guess I just wonder if there could be a single function to create a dummy transaction based off (owner_pa, token, amount, ...) (potentially having correct anchors) that can be:
- constructed (from the event) and used by clients to update their shielded state
- constructed and used by the transfer context to update undated balances and note commitment tree (somehow using the
apply_shielded_transferfunction) - constructed by the MASP VP (from the event) and fed into the existing verification logic in a way that minimizes the number of checks that are disabled/the number of paths through the verification logic.
A unified dummy transaction logic would ensure that IBC transactions are synchronized, executed, and verified in the exactly same way. Overall the code is likely correct, but it would be nice to leverage existing mainnet code as much as possible to minimize risk of unexpected behaviour. Of course, this hypothetical approach might not be practical!
Describe your changes
Closes #4830
Closes #4168
Allow IBC deposits into payment addresses (without generating MASP transactions), and IBC withdraws from payment addresses (to automatically get refunded, if an IBC unshielding transfer fails on the counter-party).
These changes should substantially improve the UX of MASP over IBC.
Checklist before merging
breaking::labelsnamada-docsreponamada-indexerornamada-masp-indexer, a corresponding PR is opened in that repo