feat(proto, core, sequencer)!: permit bech32 compatible addresses#1425
feat(proto, core, sequencer)!: permit bech32 compatible addresses#1425SuperFluffy merged 4 commits intomainfrom
Conversation
75c10e2 to
c498c75
Compare
9986b69 to
ff2c948
Compare
|
Squashed and rebased ontop of recent main because of merge conflicts. |
Fraser999
left a comment
There was a problem hiding this comment.
Just a couple of tiny nitpicks.
proto/protocolapis/astria/protocol/genesis/v1alpha1/types.proto
Outdated
Show resolved
Hide resolved
| genesisTime: "" # '2023-09-22T17:22:35.092832Z' | ||
| addressPrefixes: | ||
| base: "astria" | ||
| ibcCompat: "astriacompat" |
There was a problem hiding this comment.
do these necessarily have to be different, or is it just to easily differentiate between the two address formats?
There was a problem hiding this comment.
I think it's useful for easy identification of the two formats.
There was a problem hiding this comment.
Yes indeed. This is to make identification easier. We could always first try one and then the other format, but I think this is much cleaner.
There was a problem hiding this comment.
do we need to specify ibcCompat in the genesis or would it be sufficient to just append compat onto the base prefix?
There was a problem hiding this comment.
I am actually ambivalent here. I like to explicitly spell it out, but do you think there might be a use case where we send funds back to, e.g., an "astriabridge" prefixed address? In that case it might indeed be useful to define it at the code level and instead just append compat.
joroshiba
left a comment
There was a problem hiding this comment.
Approval for infra/protos only
| // FIXME: this needs a way to determine when to use compat address | ||
| // https://github.com/astriaorg/astria/issues/1424 | ||
| use_compat_address: false, |
There was a problem hiding this comment.
we could potentially check if the denom contains the usdc string in it
There was a problem hiding this comment.
@noot Yeah, I am thinking about how to do this. Can you add that to the issue I linked? We should probably come up with a PRD.
| // whether to use a bech32-compatible format of the `.return_address` when generating | ||
| // fungible token packets (as opposed to Astria-native bech32m addresses). This is | ||
| // necessary for chains like noble which enforce a strict bech32 format. |
There was a problem hiding this comment.
noble indeed checks the counterparty return address - unless I am mistaken about terminologies.
I have summarized the findings in this issue: #1407
Do you see how I could improve the comment to make this clearer?
) ## Summary Enables bech32 compatible addresses when necessary. ## Background Some IBC enabled chains enforce that ics20 transfer packets must contain bech32 compatible only (see #1407 for more background). This patch adds a `ibc_compat` prefix to sequencer's genesis and a new field `bool Ics20Withdrawal.use_compat_address` to the withdrawal action to request a bech32 compatible address format when interacting with such chains. ## Changes - Add field `astria.protocol.transactions.v1alpha1.Ics20Withdrawal.use_compat_address` - Add field `astria.protocol.genesis.v1alpha1.AddressPrefixes.ibc_compat` - Rewrite the `Address` domain type to be generic over the format; this allows bech32 and bech32m addresses when parsing and formatting - Note that all Astria domain types containing addresses now require `Address<Bech32m>` with `Bech32m` also being set as the default type is the parameter is absent - this now enforces that all Astria addresses in Astria's domain types *must* be bech32m compatible (this was not enforced before because `bech32::decode` does not enforce either checksum as of `bech32@v0.11`). - When executing an `astria.protocol.transactions.v1alpha1.Ics20Withdrawal` with field `use_compat_address == true`, sequencer will now construct a bech32 compatible address using the value stored at `prefixes/ibc-compat` in its state. - When refunding a rollup due to a failed withdrawal, sequencer will now attempt to parse the sender as either a standard (non-compat) bech32m address, or a compat bech32 address. - If a compat bech32 address was found during a refund, sequencer will convert it to a bech32m address with its base prefix for further processing. This ensures that the compat addresses are only ever constructed for outgoing and incoming fungible token packets. ## Testing + Updated sequencer tests for ics20 refunds for both compat and base/non-compat senders. + Updated all snapshot tests + Added a snapshot test for `Address<Bech32>` + Added unit tests for address parsing (to ensure that bech32 addresses cannot be parsaed as bech32m and vice versa) + Updated chart sequencer genesis to run smoke tests + IBC tests for noble are still work in progress and outside the scope of this PR. ## Breaking Changelist - Sequencer gained a new genesis value (the ibc compat prefix), changing the app hash ## Related Issues Closes #1407
* main: feat(sequencer): make mempool balance aware (#1408) chore(sequencer): change test addresses to versions with known private keys (#1487) chore(chart): update geth tag (#1485) feat(sequencer): report deposit events (#1447) feat(proto, core, sequencer)!: add traceability to rollup deposits (#1410) fix(bridge-withdrawer, cli, sequencer-client): migrate from `broadcast_tx_commit` to `broadcast_tx_sync` (#1376) fix(sequencer): add `end_block` to `app_execute_transaction_with_every_action_snapshot` (#1455) release: end of iteration release cuts (#1456) chore(charts): rollupName templates (#1458) chore(sequencer-relayer): Add instrumentation (#1375) feat(proto, core, sequencer)!: permit bech32 compatible addresses (#1425) chore: memoize `address_bytes` of verification key (#1444) chore(ci): include `ibc-bridge-test` in `docker` CI target (#1438) chore(charts): bump celestia versions (#1431)
Summary
Enables bech32 compatible addresses when necessary.
Background
Some IBC enabled chains enforce that ics20 transfer packets must contain bech32 compatible only (see #1407 for more background). This patch adds a
ibc_compatprefix to sequencer's genesis and a new fieldbool Ics20Withdrawal.use_compat_addressto the withdrawal action to request a bech32 compatible address format when interacting with such chains.Changes
astria.protocol.transactions.v1alpha1.Ics20Withdrawal.use_compat_addressastria.protocol.genesis.v1alpha1.AddressPrefixes.ibc_compatAddressdomain type to be generic over the format; this allows bech32 and bech32m addresses when parsing and formattingAddress<Bech32m>withBech32malso being set as the default type is the parameter is absentbech32::decodedoes not enforce either checksum as ofbech32@v0.11).astria.protocol.transactions.v1alpha1.Ics20Withdrawalwith fielduse_compat_address == true, sequencer will now construct a bech32 compatible address using the value stored atprefixes/ibc-compatin its state.Testing
Address<Bech32>Breaking Changelist
Related Issues
Closes #1407