refactor(core, proto)!: define bridge memos in proto by SuperFluffy · Pull Request #1285 · astriaorg/astria

SuperFluffy · 2024-07-22T15:57:53Z

Summary

Defines the bridge unlock, ics20 withdrawal, and ics20 deposit memos as protobuf.

Background

The various memos were read inside sequencer to inform the behaviour of the protocol, but were not defined in the canonical astria.protocol.v1alpha1 protobufs. This patch defines the memo types in a new astria.protocol.v1alpha1.memos so that there is a single source of truth for all protocol-relevant objects. The memos are still only ever serialiazed as plaintext JSON and not as protobuf.

Changes

Define the following types:
- astria.protocol.v1alpha1.memos.BridgeUnlock
- astria.protocol.v1alpha1.memos.Ics20WithdrawalFromRollup
- astria.protocol.v1alpha1.memos.Ics20TransferDeposit
Remove the astria_core::bridge module
Update astria_bridge_contracts, astria_bridge_withdrawer, and astria_sequencer to operate in terms of these types
Update all memo fields containing bytes to be of type string so that the rollup-native formatting can be used (to make it easier to identify e.g. transactions and avoid the use of base64 for non-Astria byte slices)
Various field names have been updated to clarify that their source is the rollup (e.g. rollup_transaction_hash, rollup_return_address).

Testing

The changes in this code rely on smoke tests still working. Because the IBC tests require the latest (rust) astria-cli, the IBC tests were reworked.

Breaking Changelist

This is a protocl breaking change: the shape of the memo JSON objects has changed (object keys were renamed, the encoding of byte fields no longer requires base64).

Related Issues

Closes #1286

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

proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto

joroshiba · 2024-07-25T00:46:30Z

proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto

It seems as though the only breaking change here is changing the strings used for the memos. Would be good to have some discussion over this. I'm not tied to what they are, it's good to have them be designed more.

Technically it is better to have as concise as necessary field names here, as each charachter is bytes we must include in txs and thus blocks, and we should expect briding txs to be some of our absolute most common txs.

I have changed the format of the hash and address types: before, the rust types were of type [u8; N] and serialized as base64, now they are opaque string so that the rollup can decide to write them in its favorite format.

No hard opinions on the names. I think this would be better addressed by compression of the data before being put into blocks, but maybe that would require too much of a lift. :-/

I think a long term option would be that we could upgrade to accept json OR base64 formatted pb wire format through the memo. Less human readable, would offer the wire format elimination of names space savings.

Reviewing names, I think these do offer enhanced clarity.

SuperFluffy · 2024-07-25T17:25:04Z

.github/workflows/docker-build.yml

+      - name: Install astria cli (rust)
+        run: just install-cli
+      - name: Fetch and install celestia-appd
+        run: just get-celestia-appd v1.9.0 Linux x86_64


This could be avoided if we make celestia-local into a stateful set. We could then kubectl execute from inside the celestia-local container, making this step unnecessary.

I'll note that celestia-local being a statefulset is technically correct and I believe basically a oneliner + file rename for consistency.

Some concern that this creates room for key collision running locally. I think we can refactor the smoke-test in general after merge though.

Yes, this indeed has that issue, which running directly inside a stateful set wouldn't have. Followup sounds good. I will create an issue.

SuperFluffy · 2024-07-25T17:25:46Z

.github/workflows/docker-build.yml

+          just ibc-test deploy $TAG
+      - name: Run IBC ICS20 Transfer test
+        timeout-minutes: 3
+        run: just ibc-test run ./celestia ./celestia-appd


Similar to above: the arguments are required because we need to provide a celestia "home" directory, and the path to the celestia-appd executable. It would be neater if we could run straight from the container running celestia-appd.

joroshiba

Approved, I think in general the smoke tests could use cleanup but that can be follow up.

joroshiba · 2024-07-25T17:44:52Z

charts/deploy.just

+init-ibc-bridge privateKey asset feeAsset rollupName=defaultRollupName:
+  astria-cli sequencer init-bridge-account \
+    --rollup-name {{ rollupName }} \
+    --private-key {{ privateKey }} \
+    --sequencer.chain-id {{ sequencer_chain_id }} \
+    --sequencer-url {{ sequencer_rpc_url }} \
+    --fee-asset {{ feeAsset }} \
+    --asset {{ asset }}


nit: duplication with above init-rollup-bridge code. this can be follow up cleanup though.

It's a partial duplication. feeAsset and asset are hardcoded in init-rollup-bridge, and there is an additional lock command to lock to the rollup. Could be refactored.

charts/ibc-test.just

joroshiba · 2024-07-25T17:55:01Z

justfile

+# operatingSystem is Linux or Darwin
+# machineHardwareName is arm64 or x86_64
+celestia_default_appd_dst := "."
+get-celestia-appd version operatingSystem machineHardwareName dst=celestia_default_appd_dst:


we have install-cli should this be install-celestia-appd?

No, because we don't install it. cargo install foo puts foo in a well known spot (~/.cargo/bin) that is expexted to be in PATH. I don't really know where to put this binary because I don't have knowledge of the users PATH.

Either way, we can remove this entirely when moving to a stateful set.

joroshiba · 2024-07-25T17:55:55Z

.github/workflows/docker-build.yml

+      - name: Install astria cli (rust)
+        run: just install-cli
+      - name: Fetch and install celestia-appd
+        run: just get-celestia-appd v1.9.0 Linux x86_64


I'll note that celestia-local being a statefulset is technically correct and I believe basically a oneliner + file rename for consistency.

Some concern that this creates room for key collision running locally. I think we can refactor the smoke-test in general after merge though.

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

joroshiba · 2024-07-25T18:11:43Z

proto/protocolapis/astria/protocol/memos/v1alpha1/types.proto

I think a long term option would be that we could upgrade to accept json OR base64 formatted pb wire format through the memo. Less human readable, would offer the wire format elimination of names space savings.

Reviewing names, I think these do offer enhanced clarity.

Co-authored-by: Jordan Oroshiba <jordan@astria.org>

* main: release: cut bridge withdrawer release (#1303) release: version cuts for dusk-9 (#1299) chore(core): remove ed25519_consensus from public API (#1277) chore: remove spurious entry in gitignore (#1276) chore(chart): Update EVM-Rollup Geth devTag (#1300) fix(proto)!: Change execution API to use primitive RollupId (#1291) refactor(core, proto)!: define bridge memos in proto (#1285) chore(sequencer-relayer)!: minimize resubmissions to Celestia (#1234)

…1302) ## Summary Updates the `charts/celestia-local` to spin up `celestia-appd` in a stateful set. Executes the ics20 transfer inside the celestia-app container inside the set. ## Background #1285 updates the ibc ics20 transfer test to require `celestia-appd` and an initialized celestia keystore to be present on the machine running the test. This is undesirable because it pollutes the host environment and creates potential for collisions. Turning `celestia-local` from a kubernetes deployment to a stateful set allows executing the ics20 transfer from inside the container. ## Changes - Rename `charts/celestia-local/templates/deployment.yaml -> charts/celestia-local/templates/statefulsets.yaml` - Changes `kind: Deployment -> kind: StatefulSet` in that file - Update recipes in `charts/deploy.just` to use `kubectl rollout status` instead of `kubectl wait` (the latter does not seem to work with stateful sets) - Use `kubectl exec` against the celestia-app container in the stateful set to initialize the transfer instead of a locally present `celestia-appd` ## Testing This is an update to the ibc ics20 smoke test flow. The test still passes with the expected result. ## Related Issues Closes #1296 Closes #1295

## Summary Defines `astria.protocol.genesis.v1alpha1.GenesisAppState`, replacing the Rust-as-JSON definition. ## Background All protocol relevant Astria types are supposed to be defined in Astria's protobuf spec. With #1285 having redefined the memos as protobuf message, this patch migrates the last type to protobuf spec. ## Changes - Define `astria.protocol.genesis.v1alpha1.GenesisAppState` and related protobuf messages - Remove the `astria-core::sequencer::GenesisState` module - Update Sequencer in terms of the protobuf type - Update all charts and snapshots (especially the genesis template) ## Testing All tests have been updated to use the new types, including snapshot tests. The genesis state is only read at `init-chain` and does not affect the evaluation of the state machine. Hence no tests should change as long as the same data is passed in (which is reflected by `tests-breaking-changes` leading to the same hash). ## Breaking Changelist This is a breaking change because an old sequencer would not understand a new (json-formatted) genesis app state and vise versa. ## Related issues Closes #1347

…(#1302) ## Summary Updates the `charts/celestia-local` to spin up `celestia-appd` in a stateful set. Executes the ics20 transfer inside the celestia-app container inside the set. ## Background astriaorg/astria#1285 updates the ibc ics20 transfer test to require `celestia-appd` and an initialized celestia keystore to be present on the machine running the test. This is undesirable because it pollutes the host environment and creates potential for collisions. Turning `celestia-local` from a kubernetes deployment to a stateful set allows executing the ics20 transfer from inside the container. ## Changes - Rename `charts/celestia-local/templates/deployment.yaml -> charts/celestia-local/templates/statefulsets.yaml` - Changes `kind: Deployment -> kind: StatefulSet` in that file - Update recipes in `charts/deploy.just` to use `kubectl rollout status` instead of `kubectl wait` (the latter does not seem to work with stateful sets) - Use `kubectl exec` against the celestia-app container in the stateful set to initialize the transfer instead of a locally present `celestia-appd` ## Testing This is an update to the ibc ics20 smoke test flow. The test still passes with the expected result. ## Related Issues Closes #1296 Closes #1295

## Summary Defines `astria.protocol.genesis.v1alpha1.GenesisAppState`, replacing the Rust-as-JSON definition. ## Background All protocol relevant Astria types are supposed to be defined in Astria's protobuf spec. With astriaorg/astria#1285 having redefined the memos as protobuf message, this patch migrates the last type to protobuf spec. ## Changes - Define `astria.protocol.genesis.v1alpha1.GenesisAppState` and related protobuf messages - Remove the `astria-core::sequencer::GenesisState` module - Update Sequencer in terms of the protobuf type - Update all charts and snapshots (especially the genesis template) ## Testing All tests have been updated to use the new types, including snapshot tests. The genesis state is only read at `init-chain` and does not affect the evaluation of the state machine. Hence no tests should change as long as the same data is passed in (which is reflected by `tests-breaking-changes` leading to the same hash). ## Breaking Changelist This is a breaking change because an old sequencer would not understand a new (json-formatted) genesis app state and vise versa. ## Related issues Closes astriaorg/astria#1347

…(#1302) ## Summary Updates the `charts/celestia-local` to spin up `celestia-appd` in a stateful set. Executes the ics20 transfer inside the celestia-app container inside the set. ## Background astriaorg/astria#1285 updates the ibc ics20 transfer test to require `celestia-appd` and an initialized celestia keystore to be present on the machine running the test. This is undesirable because it pollutes the host environment and creates potential for collisions. Turning `celestia-local` from a kubernetes deployment to a stateful set allows executing the ics20 transfer from inside the container. ## Changes - Rename `charts/celestia-local/templates/deployment.yaml -> charts/celestia-local/templates/statefulsets.yaml` - Changes `kind: Deployment -> kind: StatefulSet` in that file - Update recipes in `charts/deploy.just` to use `kubectl rollout status` instead of `kubectl wait` (the latter does not seem to work with stateful sets) - Use `kubectl exec` against the celestia-app container in the stateful set to initialize the transfer instead of a locally present `celestia-appd` ## Testing This is an update to the ibc ics20 smoke test flow. The test still passes with the expected result. ## Related Issues Closes #1296 Closes #1295

## Summary Defines `astria.protocol.genesis.v1alpha1.GenesisAppState`, replacing the Rust-as-JSON definition. ## Background All protocol relevant Astria types are supposed to be defined in Astria's protobuf spec. With astriaorg/astria#1285 having redefined the memos as protobuf message, this patch migrates the last type to protobuf spec. ## Changes - Define `astria.protocol.genesis.v1alpha1.GenesisAppState` and related protobuf messages - Remove the `astria-core::sequencer::GenesisState` module - Update Sequencer in terms of the protobuf type - Update all charts and snapshots (especially the genesis template) ## Testing All tests have been updated to use the new types, including snapshot tests. The genesis state is only read at `init-chain` and does not affect the evaluation of the state machine. Hence no tests should change as long as the same data is passed in (which is reflected by `tests-breaking-changes` leading to the same hash). ## Breaking Changelist This is a breaking change because an old sequencer would not understand a new (json-formatted) genesis app state and vise versa. ## Related issues Closes astriaorg/astria#1347

github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels Jul 22, 2024

SuperFluffy marked this pull request as ready for review July 22, 2024 15:58

SuperFluffy requested review from a team as code owners July 22, 2024 15:58

SuperFluffy temporarily deployed to BUF July 22, 2024 15:58 — with GitHub Actions Inactive

SuperFluffy requested review from joroshiba and noot as code owners July 22, 2024 15:58

SuperFluffy force-pushed the superfluffy/memo-into-proto branch from 570dcf2 to 7efa2b1 Compare July 22, 2024 16:05

SuperFluffy temporarily deployed to BUF July 22, 2024 16:06 — with GitHub Actions Inactive

SuperFluffy force-pushed the superfluffy/memo-into-proto branch from 7efa2b1 to dcfe19a Compare July 22, 2024 16:09

SuperFluffy temporarily deployed to BUF July 22, 2024 16:09 — with GitHub Actions Inactive

SuperFluffy force-pushed the superfluffy/memo-into-proto branch from dcfe19a to 6ad69f5 Compare July 22, 2024 16:12

SuperFluffy temporarily deployed to BUF July 22, 2024 16:12 — with GitHub Actions Inactive

joroshiba reviewed Jul 25, 2024

View reviewed changes

refactor(core, proto)!: define bridge memos in proto

0c5be83

SuperFluffy force-pushed the superfluffy/memo-into-proto branch from 6ad69f5 to ba836bd Compare July 25, 2024 13:50

SuperFluffy requested review from a team as code owners July 25, 2024 13:50

SuperFluffy requested a review from steezeburger July 25, 2024 13:50

github-actions bot added ci issues that are related to ci and github workflows cd labels Jul 25, 2024

SuperFluffy temporarily deployed to BUF July 25, 2024 13:50 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF July 25, 2024 15:48 — with GitHub Actions Inactive

SuperFluffy added 3 commits July 25, 2024 19:15

rework tests to use astria-cli-rs instead of astria-cli-go

8921832

update github workflow

a6495a8

remove no longer necessary charts

8927a93

SuperFluffy force-pushed the superfluffy/memo-into-proto branch from fdfd94b to 8927a93 Compare July 25, 2024 17:22

SuperFluffy temporarily deployed to BUF July 25, 2024 17:23 — with GitHub Actions Inactive

SuperFluffy commented Jul 25, 2024

View reviewed changes

SuperFluffy added 2 commits July 25, 2024 19:49

add back snapshot tests for proto defined json memos

a3ee474

add comments on the memo protos

b55fa71

SuperFluffy temporarily deployed to BUF July 25, 2024 17:57 — with GitHub Actions Inactive

SuperFluffy requested a review from joroshiba July 25, 2024 18:01

joroshiba approved these changes Jul 25, 2024

View reviewed changes

clarify error message for unset deposit address

b83d6c5

SuperFluffy temporarily deployed to BUF July 25, 2024 18:36 — with GitHub Actions Inactive

fix comment explaining mult factor

c3d86d0

Co-authored-by: Jordan Oroshiba <jordan@astria.org>

SuperFluffy temporarily deployed to BUF July 25, 2024 18:39 — with GitHub Actions Inactive

SuperFluffy enabled auto-merge July 25, 2024 18:42

SuperFluffy added this pull request to the merge queue Jul 25, 2024

Merged via the queue into main with commit 892e408 Jul 25, 2024

SuperFluffy deleted the superfluffy/memo-into-proto branch July 25, 2024 18:54

This was referenced Jul 25, 2024

IBC smoke tests shouldn't require an additional binary to install #1295

Closed

refactor(ci): execute ics20 test from statefulset celestia container #1302

Merged

SuperFluffy mentioned this pull request Aug 4, 2024

refactor(core, proto)!: define app genesis state in proto #1346

Merged

Conversation

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

Uh oh!

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Jul 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SuperFluffy commented Jul 22, 2024 •

edited

Loading

SuperFluffy Jul 25, 2024 •

edited

Loading