Skip to content
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

fix(bridge-withdrawer): check if base denom is usdc to determine when to use compat address #1671

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

noot
Copy link
Collaborator

@noot noot commented Oct 16, 2024

Summary

check if base denom is usdc to determine when to use compat address as the sender/return address.

Background

required for if the withdrawal to noble times out/fails

Changes

  • check if base denom is usdc, if so, set use_compat_address to true, false otherwise.

Testing

existing unit tests, would need to have a withdrawal timeout test for noble to fully test this

Related Issues

closes #1424

@noot noot requested a review from a team as a code owner October 16, 2024 18:02
@noot noot requested a review from SuperFluffy October 16, 2024 18:02
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think astria-bridge-contracts is the wrong place to decide if the compat address should be used or not. For one, what if there are other denominations that require a compat address? Also, what if I want to send wrapped usdc to another chain that is not noble (and capable of dealing with bech32m)?

The contracts crate should allow specifying compatibility mode, but through its builder not via a heuristic.

Instead, let's move this check to astria-bridge-withdrawer - and provide a new flag for the astria-cli bridge collect-withdrawals subcommand.

//
// since we're always unwrapping the asset (sending back to the originating chain), we
// can check that the base denom is USDC, and if it is, we know we're sending to Noble.
use_compat_address = ics20_asset_to_withdraw.base_denom() == "usdc";
Copy link
Member

@SuperFluffy SuperFluffy Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoding this here, I'd prefer to add a method fn use_compat_address(self, use_compat_address: bool) -> Self on the builder and let the user of this crate decide if they want compat or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bridge withdrawer, contracts, CLI need to understand when to demand compat addresses
2 participants