Conversation
SuperFluffy
reviewed
Aug 15, 2024
Contributor
SuperFluffy
left a comment
There was a problem hiding this comment.
Ready to approve, but I think you can make the change to conductor non-breaking.
ethanoroshiba
left a comment
There was a problem hiding this comment.
LGTM, but I tend to agree with Janis in wondering if we could make the changes non-breaking. Would it be possible to instantiate celestia_token here based on whether cfg.celestia_bearer_token contains a value or not, rather than using cfg.no_celestia_bearer_token?
SuperFluffy
reviewed
Aug 21, 2024
| pub celestia_node_http_url: String, | ||
|
|
||
| /// Disables using the bearer token auth header for the Celestia jsonrpc | ||
| pub no_celestia_auth: bool, |
Contributor
There was a problem hiding this comment.
This needs to get a default value because otherwise this forces a user to always set ASTRIA_CONDUCTOR_NO_CELESTIA_AUTH=false which I think is highly confusing:
Suggested change
| pub no_celestia_auth: bool, | |
| #[serde(default)] | |
| pub no_celestia_auth: bool, |
See this error message:
failed to start conductor:
{"0": "failed reading config", "1": "failed reading config from process environment", "2": "missing field `no_celestia_auth`"}
EDIT: I correct myself, this is in line with how we do everything else.
SuperFluffy
approved these changes
Aug 21, 2024
steezeburger
added a commit
that referenced
this pull request
Aug 22, 2024
* 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)
Fraser999
pushed a commit
to Fraser999/astria
that referenced
this pull request
Aug 26, 2024
## Summary Updates conductor celestia client to support using no auth token, adds a config field to specify using no token. ## Background When originally built celestia-node required an auth token, it can now be run with the auth token disabled. We do this by default in our charts, but our code always specifies an auth header which will still be rejected if empty by celestia node. ## Changes - Add `ASTRIA_CONDUCTOR_NO_CELESTIA_AUTH` config env var - When `no_celestia_auth` is true, makes celestia node requests without auth header. ## Testing CI/CD smoke tests use no token, blockbox tests use a token to verify both paths still work. ## Breaking Changelist - ASTRIA_CONDUCTOR_NO_CELESTIA_AUTH config env var added ## Related Issues closes astriaorg#1370
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
Updates conductor celestia client to support using no auth token, adds a config field to specify using no token.
Background
When originally built celestia-node required an auth token, it can now be run with the auth token disabled. We do this by default in our charts, but our code always specifies an auth header which will still be rejected if empty by celestia node.
Changes
ASTRIA_CONDUCTOR_NO_CELESTIA_AUTHconfig env varno_celestia_authis true, makes celestia node requests without auth header.Testing
CI/CD smoke tests use no token, blockbox tests use a token to verify both paths still work.
Breaking Changelist
Related Issues
closes #1370