feat(sequencer)!: allow configuring base address prefix by SuperFluffy · Pull Request #1201 · astriaorg/astria

SuperFluffy · 2024-06-20T16:18:39Z

Summary

This patch allows configuring the base address prefix in sequencer's genesis file and enforces this prefix in all actions.

Similar to the native fee, the base prefix is set once during init-chain and is then available globally and never changed.

Background

#1124 changed astria addresses from opaque bytes to bech32m addresses, which themselves are encoded as strings on the wire. Where previously sequencer implicitly assumed that all addresses were "astria" prefixed bech32m addresses, this is now enforced.

Changes

Update sequencer GenesisState to use the canonical serde implementation of all astria-core types.
Add a field .address_prefixes.base to the GenesisState.
Ensure that all addresses in GenesisState are consistenst with the prefix.
Inject the genesis base address prefix into sequencer's global state.
Add a check for the global address prefix to all sequencer actions that carry a astria.primitive.v1.core.Address (this is done via AppHandler::check_stateless). These are specifically:
- TransferAction
- SudoAddressChangeAction
- BridgeLockAction
- BridgeSudoChangeAction
- BridgeUnlockAction
- InitBridgeAccountAction
- IbcRelayerChangeAction
Slightly change all storage keys that use addresses to be explicit about using bytes and provide snapshot tests (specifically in accounts::state_ext, bridge::state_ext, ibc::state_ext)
Removed: unsigned transaction parameters no longer require a bech32m chain ID name
Update composer, bridge-withdrawer to have a configurable address prefix:
- ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_ADDRESS_PREFIX
- ASTRIA_COMPOSER_SEQUENCER_ADDRESS_PREFIX
Update all astria-cli commands to take a configurable prefix.

Testing

Update all tests and ensure they still run.
Add tests for genesis construction.
Add genesis snapshot tests.
Add snapshots tests for IBC denoms (to accompany the genesis snapshot)

Breaking Changelist

This change is breaking at the network level requiring sequencer re-genesis. Non-bech32m addresses in actions are now rejected, as well as addresses that have a prefix other then the one set at genesis. In addition, the storage key for all collections involving an address have changed.

crates/astria-sequencer/src/accounts/state_ext.rs

noot · 2024-06-24T17:59:44Z

crates/astria-bridge-withdrawer/src/bridge_withdrawer/ethereum/convert.rs

    Ok(Action::BridgeUnlock(action))
 }

+// FIXME: Get this to work for now, but replace this with a builder.


i'm refactoring/removing this function in my next PR, so this is fine to leave

crates/astria-bridge-withdrawer/src/bridge_withdrawer/submitter/signer.rs

crates/astria-core/src/primitive/v1/asset/denom.rs

crates/astria-sequencer/src/app/tests_breaking_changes.rs

crates/astria-sequencer/src/genesis.rs

noot

looks good to me!

joroshiba

A general note on composer & withdrawer is that we could get the prefix from genesis fetch.

Some nits through out but mostly looks good, didn't comment on each repeat but the stateless checks all just say "invalid" when they are specifically invalid because of wrong prefix.

Note that the ibc name stuff seems unrelated, is that artifact from other work?

crates/astria-cli/src/cli/sequencer.rs

crates/astria-core/src/primitive/v1/asset/denom.rs

...quencer/src/accounts/snapshots/astria_sequencer__accounts__state_ext__test__snapshots-2.snap

crates/astria-sequencer/src/accounts/state_ext.rs

crates/astria-sequencer/src/address/mod.rs

crates/astria-sequencer/src/authority/action.rs

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs

crates/astria-sequencer/src/bridge/state_ext.rs

crates/astria-sequencer/src/ibc/state_ext.rs

crates/astria-cli/src/cli/sequencer.rs

SuperFluffy · 2024-06-24T21:45:02Z

...uencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap

This and the other snapshots have changed because the base prefix is now written to app storage.

noot · 2024-06-25T17:07:31Z

charts/sequencer/values.yaml

-      # NOTE - the following address matches the privKey that funds the sequencer-faucet
-    - address: 00d75b270542084a54fcf0d0f6eab0402982d156
-      balance: "333333333333333333"
+      - address: 1c0c490f1b5528d8173c5de46d131160e4b2c0c3


just to confirm - does the genesis support both hex and bech32 now?

So the chart is indifferent, when you run in dev mode it builds the old genesis that is intended to work with the latest release. When you run in dev mode it builds the new genesis which works on this PR and latest after this is merged.

Yeah to chime in: this is only for CI. The sequencer genesis itself only supports proper bech32m addresses.

joroshiba · 2024-06-25T20:43:28Z

crates/astria-sequencer/src/address/mod.rs

+    Ok(())
+}
+
+#[cfg(not(test))]


Can we get a follow up issue?

SuperFluffy · 2024-06-26T13:40:01Z

@joroshiba #1208 for a follow-up for the cfg(not(test)) business. It doens't explicitly mention it, but that change would make the compile-time shenanigans unnecessary.

…ddresses

…napshots

Updates chart to use a helper and use single value instead of double value setup.

…orted

github-actions bot added sequencer pertaining to the astria-sequencer crate composer pertaining to composer labels Jun 20, 2024

SuperFluffy marked this pull request as ready for review June 20, 2024 16:36

SuperFluffy requested review from a team, joroshiba and noot as code owners June 20, 2024 16:36

SuperFluffy added the bridging label Jun 20, 2024

SuperFluffy changed the title ~~feat(sequencer!): allow configuring base address prefix~~ feat(sequencer)!: allow configuring base address prefix Jun 20, 2024

SuperFluffy commented Jun 20, 2024

View reviewed changes

crates/astria-sequencer/src/accounts/state_ext.rs Outdated Show resolved Hide resolved

SuperFluffy requested a review from a team as a code owner June 21, 2024 13:47

github-actions bot added the cd label Jun 21, 2024

SuperFluffy force-pushed the superfluffy/bech32-aware-sequencer branch from 5efa37c to 3c67150 Compare June 21, 2024 21:21

noot reviewed Jun 24, 2024

View reviewed changes

crates/astria-bridge-withdrawer/src/bridge_withdrawer/submitter/signer.rs Outdated Show resolved Hide resolved

noot reviewed Jun 24, 2024

View reviewed changes

crates/astria-core/src/primitive/v1/asset/denom.rs Show resolved Hide resolved

noot reviewed Jun 24, 2024

View reviewed changes

crates/astria-sequencer/src/app/tests_breaking_changes.rs Outdated Show resolved Hide resolved

noot reviewed Jun 24, 2024

View reviewed changes

crates/astria-sequencer/src/genesis.rs Outdated Show resolved Hide resolved

noot approved these changes Jun 24, 2024

View reviewed changes

joroshiba reviewed Jun 24, 2024

View reviewed changes

SuperFluffy enabled auto-merge June 24, 2024 20:32

joroshiba reviewed Jun 24, 2024

View reviewed changes

crates/astria-cli/src/cli/sequencer.rs Show resolved Hide resolved

SuperFluffy disabled auto-merge June 24, 2024 20:54

SuperFluffy commented Jun 24, 2024

View reviewed changes

noot reviewed Jun 25, 2024

View reviewed changes

joroshiba approved these changes Jun 25, 2024

View reviewed changes

crates/astria-sequencer/src/address/mod.rs

Ok(())

}

#[cfg(not(test))]

Copy link

Member

joroshiba Jun 25, 2024

Choose a reason for hiding this comment

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

Can we get a follow up issue?

SuperFluffy mentioned this pull request Jun 26, 2024

Remove global state in sequencer #1208

Closed

SuperFluffy added this pull request to the merge queue Jun 26, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 26, 2024

WIP

7c9e18a

SuperFluffy and others added 24 commits June 26, 2024 15:54

remove bridge withdrawer dependence on core const

33fb78a

remove sequencer dependence on core const

2d0eed0

remove sequencer0client dependence on core const

36c6e82

make prefix configurable for all astria cli commands that construct a…

2642f51

…ddresses

use address bytes without prefix to construct storage keys; provide s…

8c033cf

…napshots

fix tests

125f243

fix racy sequencer address prefix setter

fe2e085

add subcommand to construct bech32m addresses from hex bytes and prefix

c8959a3

WIP: update charts

aecdb85

bump evm chart

b26bd34

add address prefixes to values.yaml

cf99150

fix incorrectly named field in genesis

b256e44

fix evm-rollup charts

0f6ed18

chore: minor updates to in flight pr chart (#1204)

1b937f0

Updates chart to use a helper and use single value instead of double value setup.

typos

b1df3ce

use a better error message to state that address prefixes were unsupp…

da36830

…orted

more descriptive names for genesis, key snapshots

0fe10b8

bring back renamed snapshot tests

6a169f7

use astria as the default prefix for the bech32m cli tool

4751c5b

write address base prefix into state

22a5f81

use full address in sequencer-client extension trait

34b83cc

provide astria as the default prefix for all cli args

6313006

expand base prefix global getter/setter to mention sequencer restart

c917e93

update after rebase

435c9ac

SuperFluffy force-pushed the superfluffy/bech32-aware-sequencer branch from cf7437d to 435c9ac Compare June 26, 2024 13:55

SuperFluffy enabled auto-merge June 26, 2024 13:56

SuperFluffy added this pull request to the merge queue Jun 26, 2024

Merged via the queue into main with commit d35271d Jun 26, 2024

SuperFluffy deleted the superfluffy/bech32-aware-sequencer branch June 26, 2024 14:09

Conversation

SuperFluffy commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Changes

Testing

Breaking Changelist

Uh oh!

Uh oh!

noot Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noot left a comment

Choose a reason for hiding this comment

Uh oh!

joroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SuperFluffy Jun 24, 2024

Choose a reason for hiding this comment

Uh oh!

noot Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

joroshiba Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

joroshiba Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

SuperFluffy commented Jun 26, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SuperFluffy commented Jun 20, 2024 •

edited

Loading

noot Jun 24, 2024 •

edited

Loading