chore(conductor, composer)!: update chain_id strings to rollup_name#633
Closed
joroshiba wants to merge 6 commits into
Closed
chore(conductor, composer)!: update chain_id strings to rollup_name#633joroshiba wants to merge 6 commits into
joroshiba wants to merge 6 commits into
Conversation
github-actions
Bot
added
conductor
joroshiba
changed the title
SuperFluffy
approved these changes
Dec 4, 2023
SuperFluffy
left a comment
SuperFluffy
left a comment
Contributor
There was a problem hiding this comment.
Good follow-up to tie up loose ends after #436 got merged.
noot
reviewed
Dec 5, 2023
noot
reviewed
Dec 5, 2023
github-actions
Bot
added
sequencer
SuperFluffy
approved these changes
Dec 7, 2023
SuperFluffy
left a comment
SuperFluffy
left a comment
Contributor
There was a problem hiding this comment.
Minor suggestion. In general I think there are likely other places where our variable naming is not perfect with respect to chain Id, rollup ID, and name. But I think that's ok.
| return Err(e).wrap_err_with(|| { | ||
| format!( | ||
| "collector for chain ID {chain_id} failed while waiting for it to become \ | ||
| "collector for rollup {rollup_name} failed while waiting for it to become \ |
Contributor
There was a problem hiding this comment.
Suggested change
| "collector for rollup {rollup_name} failed while waiting for it to become \ | |
| "collector for rollup `{rollup_name}` failed while waiting for it to become \ |
I like doing this to have the name stand out a bit more.
noot
approved these changes
Dec 12, 2023
sambukowski
approved these changes
Dec 21, 2023
Member
Author
|
Closing in favor of #791 due to staleness and many changes since put together. |
joroshiba
added a commit
that referenced
this pull request
Mar 13, 2024
## Summary Disambiguate uses of chain-id in core services, making sure it's clear when we mean `rollup-id`, `rollup-name`, or `chain-id` ## Background We previously used to have a lot of chain id overlap. Now chain id (outside of the cli crate), should refer to only the cometbft chain id of either the sequencer or another IBC compatible chain id. Instances referring to rollups are now referenced wholly by `rollup-name` when it is a string or `rollup-id` when it is a 32 bit byte array. There is a previous stale PR (#633) that accomplished this, but many change since then opted to go from scratch. ## Changes - update references mostly in composer to `chain-id`
sgranfield4403-3
added a commit
to sgranfield4403-3/astria
that referenced
this pull request
Oct 2, 2025
## Summary Disambiguate uses of chain-id in core services, making sure it's clear when we mean `rollup-id`, `rollup-name`, or `chain-id` ## Background We previously used to have a lot of chain id overlap. Now chain id (outside of the cli crate), should refer to only the cometbft chain id of either the sequencer or another IBC compatible chain id. Instances referring to rollups are now referenced wholly by `rollup-name` when it is a string or `rollup-id` when it is a 32 bit byte array. There is a previous stale PR (astriaorg/astria#633) that accomplished this, but many change since then opted to go from scratch. ## Changes - update references mostly in composer to `chain-id`
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
4 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
changed references to
chain_idthat were strings torollup_nameorrollup_idfrom the sequencer viewpoint.Background
Chain ID is hugely overloaded, we previously updated our protos, but we haven't updated some configuration to remove the overloading.
Changes
Breaking Changes