feat(sequencer)!: implement bridge sudo and withdrawer addresses #1142
Conversation
github-actions
bot
added
proto
noot
changed the title
| // the address corresponding to the key which has sudo capabilities; | ||
| // ie. can change the sudo and withdrawer addresses for this bridge account. | ||
| astria.primitive.v1.Address sudo_address = 4; | ||
| // the address corresponding to the key which can withdraw funds from this bridge account. |
There was a problem hiding this comment.
Can we note in comments what happens if they are left a null?
| @@ -27,6 +27,7 @@ | |||
| "sequence_byte_cost_multiplier": 1, | |||
| "init_bridge_account_base_fee": 48, | |||
| "bridge_lock_byte_cost_multiplier": 1, | |||
| "bridge_sudo_change_fee": 24, | |||
There was a problem hiding this comment.
does this make this a hard fork requiring network regen change?
In general actually, we probably need an answer for this. We should be able to add actions with fees without regenesis of network
There was a problem hiding this comment.
yes, this would be a hard fork. without hard fork logic this is a regenesis
| // the new withdrawer address; unchanged if unset | ||
| astria.primitive.v1.Address new_withdrawer_address = 3; | ||
| // the asset used to pay the transaction fee | ||
| bytes fee_asset_id = 4; | ||
| } | ||
|
|
||
| message FeeChangeAction { |
There was a problem hiding this comment.
I believe we need a new entry for FeeChangeAction
| /// - if the `bridge_address` field is not set | ||
| /// - if the `bridge_address` field is invalid | ||
| /// - if the `new_sudo_address` field is invalid | ||
| /// - if the `new_withdrawer_address` field is invalid |
There was a problem hiding this comment.
fee_asset_id can also be invalid?
| // check that the sender of this tx is the authorized withdrawer for the bridge account | ||
| let Some(withdrawer_address) = state | ||
| .get_bridge_account_withdrawer_address(&bridge_address) | ||
| .await | ||
| .context("failed to get bridge account withdrawer address")? | ||
| else { | ||
| bail!("bridge account does not have an associated withdrawer address"); | ||
| }; |
There was a problem hiding this comment.
similar to above, what about old bridge accounts and during sync this will fail for old unlocks
There was a problem hiding this comment.
same as above, user would need to set their sudo/withdrawer addrs before using
| @@ -24,6 +24,7 @@ impl Component for BridgeComponent { | |||
| async fn init_chain<S: StateWriteExt>(mut state: S, app_state: &Self::AppState) -> Result<()> { | |||
| state.put_init_bridge_account_base_fee(app_state.fees.init_bridge_account_base_fee); | |||
| state.put_bridge_lock_byte_cost_multiplier(app_state.fees.bridge_lock_byte_cost_multiplier); | |||
| state.put_bridge_sudo_change_fee(app_state.fees.bridge_sudo_change_fee); | |||
There was a problem hiding this comment.
It seems like this will break if we don't have a fee set in genesis so node could not sync with old genesis?
There was a problem hiding this comment.
yes, would need hard fork logic
There was a problem hiding this comment.
Approved, with note that this will be major breaking change, requiring regensis and a dusk-8. Moving forward if we add actions with new fees I think we should simply disallow the action to be run if a fee has not been set. This provides a mechanism through which we can add a new action without changing the genesis.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* main: fix: ignore RUSTSEC-2021-0139 (#1171) chore(sequencer-relayer)!: remove functionality to restrict relaying blocks to only those proposed by a given validator (#1168) chore(metrics): update `metric_name` macro to handle a collection of names (#1163) fix(bridge-withdrawer): skip linting generated contract code (#1172) fix(core, sequencer): prefix removal source non-refund ics20 packet (#1162) chore(docs): add sequencer-relayer doc to specs (#1126) feat(bridge-withdrawer): sync logic (#1165) chore(withdrawer): replace contracts with `astria-bridge-contracts` submodule (#1164) feat(sequencer)!: implement bridge sudo and withdrawer addresses (#1142) feat(sequencer): implement refund to rollup logic upon ics20 transfer refund (#1161) feat(bridge-withdrawer): bridge withdrawer startup (#1160) feat(core, proto)!: add bech32m addresses (#1124) feat(withdrawer): bridged ERC20 token withdrawals (#1149) feat(sequencer-relayer)!: add chain IDs for sequencer and Celestia to config env vars (#1063) test(bridge-withdrawer): add submitter tests (#1133) chore: bump penumbra deps (#1159) feat(sequencer): implement `bridge/account_last_tx_hash` abci query (#1158) fix(withdrawer): use block subscription in batcher; send to destination_chain_address (#1157) fix(withdrawer): update AstriaWithdrawer to check that withdrawal value is sufficient (#1148) chore(ci): build bridge withdrawer images (#1156)
Summary
we want bridge accounts to be "upgradable"; ie. we want to be able to change the key which can control the bridge account without changing the bridge address from the rollup's point of view. with this change, we have a sudo key and a withdrawer key for each bridge account, which is set when the bridge account is initialized. the sudo key is able to change the sudo key and withdrawer key. the withdrawer key is the only key allowed to withdraw funds from the bridge account.
Background
see above
Changes
InitBridgeAccountActionto contain asudo_addressandwithdrawer_addresswhich are set in stateBridgeUnlockActionto have abridge_addressfield which is the bridge address withdrawn from; it also checks if the signer of the tx is the bridgewithdrawer_addressand fail if notBridgeSudoChangeActionwhich allows the bridge sudo key to change the bridge sudo and/or withdrawer keysIcs20Withdrawals from bridge now also do a withdrawer key check likeBridgeUnlockAction;bridge_addresswas added to the action in the case that the withdrawer differs from the bridge addressTransferfrom a bridge account, asBridgeUnlockActionorIcs20Withdrawalmust be used.Testing
unit tests
Breaking Changelist
InitBridgeAccountstores two new keys in state: the bridge sudo key and the bridge withdrawer keyBridgeSudoChangeActionis addedNonein the native rust type.