fix(sequencer)!: allow compat prefixed addresses when receiving ics20 transfers #1655
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
quasystaty1
changed the title
SuperFluffy
changed the title
SuperFluffy
reviewed
Oct 15, 2024
There was a problem hiding this comment.
Thank you for the fix. This was an oversight.
I have changed the patch title to from feat to fix and adjusted the text slightly.
Before approving: note that at the moment we completely lack tests for ics20 transfers. Since you originally encountered this problem when trying to do ics20 transfers, can you confirm that these now work?
Better yet, is there a way to provide a smoke test for this? (I am happy with you doing a manual confirmation if adding smoke tests is too much of a lift).
quasystaty1
force-pushed
the
quasystaty1/ENG-915/ics20-transfer-support-compat
branch
from
ceac1c5
to
4cbb771
Compare
quasystaty1
force-pushed
the
quasystaty1/ENG-915/ics20-transfer-support-compat
branch
from
4cbb771
to
c529b33
Compare
noot
approved these changes
Oct 15, 2024
|
Added a smoke test |
quasystaty1
deleted the
quasystaty1/ENG-915/ics20-transfer-support-compat
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Support ics20 transfer from a counterparty to the astria sequencer.
Background
Currently, when performing an ics20 transfer to the Sequencer, the transfer only accepts bech32m address formats. This is inconsistent with refunding failed ics20 from Sequencer to the counterparty, where both compat (bech32) and normal (bech32m) addresses are accepted. Sequencer should accept both.
The main use case is when receiving assets from Noble (such as USDC). While Noble only enforces bech32 when handling failed ics20 transfers, this still requires ics20 transfers from Astria Sequencer to Noble to be bech32 (and not bech32m) encoded. The bech32 compatible address is hence the sender address that will be known by the counterparty/Noble, and we must allow transfers of funds back to it.
Changes
Testing
All test passes
Breaking Changelist
Related Issues
closes #1653