refactor(core, proto)!: define app genesis state in proto by SuperFluffy · Pull Request #1346 · astriaorg/astria

SuperFluffy · 2024-08-04T11:57:50Z

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

SuperFluffy · 2024-08-04T16:01:27Z

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

+import "astria/primitive/v1/types.proto";
+
+message GenesisAppState {
+  string chain_id = 1;


This field is in preparation of a follow up refactoring sequencer to run init-chain with only the data provided by GenesisAppState.

With this change we wouldn't additionally add a new chainId field into the genesis app_state would we?

Yeah, we'd ultimately end up doing that. I had a dislike for extra configuration data being passed in "from the outside". I would like all genesis settings relevant to the app/sequencer to be in one place.

We would then either enforce that the cometbft chain ID match the sequencer chain ID - or ignore cometbft.

The chain id is already part of the InitChain abci call, so this would be breaking expected abci conventions: https://github.com/cometbft/cometbft/blob/main/spec/abci/abci++_methods.md#initchain

This is inherently already a part of the call, I wouldn't want to override this core part of genesis definition and break the expected ABCI flow

Agree, in my mind these fields would just be duplicates: sequencer would check if its genesis contains the same chain ID that cometbft reports.

Fraser999

One tiny nitpick, otherwise LGTM.

Fraser999 · 2024-08-07T13:45:38Z

crates/astria-core/src/protocol/genesis/v1alpha1.rs

+#[cfg_attr(
+    feature = "serde",
+    derive(serde::Deserialize, serde::Serialize),
+    serde(try_from = "raw::GenesisAppState", into = "raw::GenesisAppState")


I remain not a fan of this approach, but I guess if we do decide to avoid this in the future we can always deal with this instance along with the others :)

crates/astria-core/src/protocol/genesis/v1alpha1.rs

joroshiba · 2024-08-07T19:10:01Z

charts/sequencer/files/cometbft/config/genesis.json

      {
        "address": {{ include "sequencer.address" $value.address }},
-        "balance": {{ toString $value.balance | replace "\"" "" }}
+        "balance": {{ include "sequencer.fee" ( toString $value.balance | replace "\"" "" ) }} 


This change disallows some hight initial balances that were previously possible. Limited to uint64 balances effectively.

Yes indeed. For our current use cases this is fine. A followup should fix this. I wonder if there is a way to call out to a helper script using helm?

We could have an init script which does the generation.

I have created #1388 as a followup.

charts/sequencer/templates/_helpers.tpl

joroshiba · 2024-08-07T19:12:45Z

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

+import "astria/primitive/v1/types.proto";
+
+message GenesisAppState {
+  string chain_id = 1;


With this change we wouldn't additionally add a new chainId field into the genesis app_state would we?

charts/sequencer/files/cometbft/config/genesis.json

joroshiba

Approving, with assumption as discussed offline that the test fixes for astria-cli are moved into seperate PR before merge.

SuperFluffy · 2024-08-22T12:06:12Z

Approving, with assumption as discussed offline that the test fixes for astria-cli are moved into seperate PR before merge.

Created a followup in #1392

* main: refactor(core, proto)!: define app genesis state in proto (#1346) fix(sequencer): bump penumbra dep to fix ibc state access bug (#1389) feat(conductor)!: support disabled celestia auth (#1372) fix(sequencer)!: fix block fees (#1343) perf(sequencer): add benchmark for prepare_proposal (ENG-660) (#1337) fix(proto): fix import name mismatch (#1380) fix(ci): enable bridge withdrawer building with tag (#1374) feat(sequencer): rewrite memool to have per-account transaction storage and maintenance (#1323) refactor(core, sequencer)!: require that bridge unlock address always be set (#1339) fix(sequencer)!: take funds from bridge in ics20 withdrawals (#1344) fix(sequencer)!: fix TOCTOU issues by merging check and execution (#1332) fix: abci error code (#1280) refactor(core): shorten `try_from_block_info_and_data()` (#1371) fix(relayer): change `reqwest` for `isahc` in relayer blackbox tests (ENG-699) (#1366) fix(conductor): update for celestia-node v0.15.0 (#1367) Chore: Upgrade celestia-node to v0.14.1 (#1360) chore(charts): fix charts production templates (#1359) chore(core, proto): migrate byte slices from Vec to Bytes (#1319)

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

SuperFluffy temporarily deployed to BUF August 4, 2024 11:58 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF August 4, 2024 12:06 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF August 4, 2024 13:21 — with GitHub Actions Inactive

SuperFluffy changed the title ~~Genesis as proto~~ refactor(core, proto)!: define app genesis state in proto Aug 4, 2024

SuperFluffy marked this pull request as ready for review August 4, 2024 13:32

SuperFluffy requested review from a team, joroshiba and noot as code owners August 4, 2024 13:32

SuperFluffy force-pushed the genesis-as-proto branch from e4fea3b to 44c0045 Compare August 4, 2024 13:32

SuperFluffy temporarily deployed to BUF August 4, 2024 13:33 — with GitHub Actions Inactive

SuperFluffy added the docker-build used to trigger docker builds on PRs label Aug 4, 2024

SuperFluffy marked this pull request as draft August 4, 2024 13:39

github-actions bot added the cd label Aug 4, 2024

SuperFluffy temporarily deployed to BUF August 4, 2024 14:19 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF August 4, 2024 14:24 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF August 4, 2024 14:54 — with GitHub Actions Inactive

SuperFluffy marked this pull request as ready for review August 4, 2024 15:43

SuperFluffy requested a review from a team as a code owner August 4, 2024 15:43

SuperFluffy requested a review from Fraser999 August 4, 2024 15:43

SuperFluffy temporarily deployed to BUF August 4, 2024 15:56 — with GitHub Actions Inactive

SuperFluffy commented Aug 4, 2024

View reviewed changes

Fraser999 approved these changes Aug 7, 2024

View reviewed changes

joroshiba reviewed Aug 7, 2024

View reviewed changes

SuperFluffy force-pushed the genesis-as-proto branch from 024bb29 to 116590f Compare August 21, 2024 16:28

SuperFluffy temporarily deployed to BUF August 21, 2024 16:28 — with GitHub Actions Inactive

SuperFluffy force-pushed the genesis-as-proto branch from 116590f to a74509e Compare August 21, 2024 16:40

SuperFluffy temporarily deployed to BUF August 21, 2024 16:40 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF August 21, 2024 16:58 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF August 21, 2024 17:05 — with GitHub Actions Inactive

SuperFluffy temporarily deployed to BUF August 21, 2024 17:17 — with GitHub Actions Inactive

SuperFluffy mentioned this pull request Aug 21, 2024

Allow using the entire u128 range for balances in charts #1388

Closed

SuperFluffy requested a review from joroshiba August 21, 2024 18:13

joroshiba approved these changes Aug 21, 2024

View reviewed changes

feat(proto, sequencer)!: define genesis app state in protobuf

c7e574b

SuperFluffy force-pushed the genesis-as-proto branch from 60a5022 to c7e574b Compare August 22, 2024 10:47

SuperFluffy temporarily deployed to BUF August 22, 2024 10:47 — with GitHub Actions Inactive

SuperFluffy added this pull request to the merge queue Aug 22, 2024

Merged via the queue into main with commit acff940 Aug 22, 2024

SuperFluffy deleted the genesis-as-proto branch August 22, 2024 12:17

SuperFluffy mentioned this pull request Aug 22, 2024

feat(sequencer): integrate connect oracle and vote extension logic #1236

Merged

Conversation

SuperFluffy commented Aug 4, 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!

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 Aug 20, 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!

Fraser999 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

SuperFluffy commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SuperFluffy commented Aug 4, 2024 •

edited

Loading

joroshiba Aug 20, 2024 •

edited

Loading