feat(cli, bridge-withdrawer)!: share code between cli and service#1270
Merged
SuperFluffy merged 6 commits intomainfrom Jul 15, 2024
Merged
feat(cli, bridge-withdrawer)!: share code between cli and service#1270SuperFluffy merged 6 commits intomainfrom
SuperFluffy merged 6 commits intomainfrom
Conversation
SuperFluffy
changed the title
itamarreif
reviewed
Jul 15, 2024
steezeburger
added a commit
that referenced
this pull request
Jul 19, 2024
* main: (24 commits) chore: update `bytes` and `ics23` crates (#1279) fix(sequencer): improve and fix instrumentation (#1255) feature(charts): hermes chart fixes, bech32 updates, ibc bridge test (#1130) chore(cli): remove unused rollup cli code (#1275) chore(test): use a temporary file to not pollute the workspace (#1269) chore(sequencer): add mempool benchmarks (#1238) fix(bridge-withdrawer)!: fix nonce handling (#1215) feat(cli, bridge-withdrawer)!: share code between cli and service (#1270) feat(cli): add cmd to collect withdrawal events and submit as actions (#1261) fix(core, bridge, sequencer)!: dismabiguate return addresses (#1266) fix(withdrawer): support withdrawer address that differs from bridge address (#1262) (core, sequencer)!: generate serde traits impls for all protocol protobufs (#1260) fix(charts): add resources for sequencer/cometbft (#1254) chore(sequencer)!: add metrics (#1248) fix(sequencer-utils): fixes issue in `parse_blob` tests (#1243) feat(core, proto)!: make bridge unlock memo string (#1244) fix(conductor): don't panic during panic (#1252) feat(core)!: lowerCamelCase for protobuf json mapping (#1250) refactor(bridge-withdrawer)!: refactor startup to a separate subtask and remove balance check from startup (#1190) fix: rollup archive node configurations (#1249) ...
bharath-123
pushed a commit
that referenced
this pull request
Jul 25, 2024
) ## Summary Moves common code from bridge-withdrawer and cli to `astria-bridge-contracts`. ## Background The `bridge collect-withdrawals` subcommand of `astria-cli` and the `astria-bridge-withdrawer` have common logic that should use the same code. This patch ensures that both use the same codepaths to get contract withdraw events from the astria bridge contracts running on the rollup before converting them to astria Sequencer actions. Writing this patch it was noticed that bridge-withdrawer with incorrectly set rollup asset denom would break and was fixed. ## Changes - Define a `GetWithdrawalActions::get_for_block_hash` utility for fetching withdrawal events for a given block hash and converting them to astria sequencer actions in one go - Refactor `astria-bridge-withdrawer` and `astria-cli` in terms of it - Remove `bridge collect-withdrawals --rollup-asset-denom` - Add `bridge collect-withdrawals --sequencer-asset-to-withdraw` (to disambiguate between withdrawal kinds) - Add `bridge collect-withdrawals --ics20-asset-to-withdraw` (to disambiguate between withdrawal kinds) - Require `ASTRIA_BRIDGE_WITHDRAWER_ROLLUP_ASSET_DENOM` to always contain a trace-prefixed ics20 asset denomination (a string of form `[<port>/<channel>/.../]base` because withdrawing `ibc/<hash>` denoms does not make sense for the bridge. ## Testing Smoke tests still pass and run. The overly heavy unit tests (which weren't really unit tests) in `astria-bridge-withdrawer` were removed in favor of future blackbox tests (pending in #1227, which would have removed these eventually). ## Breaking Changelist Neither of these are breaking in practice because the CLI `bridge` subcommand did not see a release yet, and `bridge-withdrawer` with `ibc/<hash>` rollup asset denoms was never used (and would likely not work) - removed an argument on the CLI - enforces a stricter config on the bridge-withdrawer
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Moves common code from bridge-withdrawer and cli to
astria-bridge-contracts.Background
The
bridge collect-withdrawalssubcommand ofastria-cliand theastria-bridge-withdrawerhave common logic that should use the same code. This patch ensures that both use the same codepaths to get contract withdraw events from the astria bridge contracts running on the rollup before converting them to astria Sequencer actions.Writing this patch it was noticed that bridge-withdrawer with incorrectly set rollup asset denom would break and was fixed.
Changes
GetWithdrawalActions::get_for_block_hashutility for fetching withdrawal events for a given block hash and converting them to astria sequencer actions in one goastria-bridge-withdrawerandastria-cliin terms of itbridge collect-withdrawals --rollup-asset-denombridge collect-withdrawals --sequencer-asset-to-withdraw(to disambiguate between withdrawal kinds)bridge collect-withdrawals --ics20-asset-to-withdraw(to disambiguate between withdrawal kinds)ASTRIA_BRIDGE_WITHDRAWER_ROLLUP_ASSET_DENOMto always contain a trace-prefixed ics20 asset denomination (a string of form[<port>/<channel>/.../]basebecause withdrawingibc/<hash>denoms does not make sense for the bridge.Testing
Smoke tests still pass and run.
The overly heavy unit tests (which weren't really unit tests) in
astria-bridge-withdrawerwere removed in favor of future blackbox tests (pending in #1227, which would have removed these eventually).Breaking Changelist
Neither of these are breaking in practice because the CLI
bridgesubcommand did not see a release yet, andbridge-withdrawerwithibc/<hash>rollup asset denoms was never used (and would likely not work)