Skip to content

Add live config preflight helpers - #42

Merged
phroi merged 7 commits into
masterfrom
review/live-config-preflight
May 27, 2026
Merged

phroi merged 7 commits into
masterfrom
review/live-config-preflight

Conversation

@phroi

@phroi phroi commented May 27, 2026

Copy link
Copy Markdown
Member

Why

Live testnet operators need a reproducible way to build ignored bot/tester configs after runtime config started allowing default RPC endpoints and retry budgets. Preflight should also show whether a config uses a custom RPC endpoint and how much plain CKB remains after the role reserve.

Changes

  • Add pnpm live:config-from-env to write ignored testnet bot/tester configs from environment variables without printing private keys or raw RPC URLs.
  • Extend the single-config generator with maxRetryableAttempts, public rpcConfigured metadata, and staged config-file installs.
  • Add preflight report fields for rpcConfigured, maxRetryableAttempts, role CKB reserve/spendable balances, and minimum deposit capital.
  • Update focused runtime config docs for bot/tester config shape.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new script ickb-live-config-from-env.mjs to generate testnet configurations from environment variables, and updates existing configuration generation and preflight scripts to support an optional rpcUrl and a new maxRetryableAttempts parameter. It also adds role-specific CKB reserves during preflight checks. The review feedback highlights critical portability issues on systems like macOS where system temporary directories are symlinked, recommending that the root path be resolved using realpath in runPreflight, runLiveConfigFromEnv, and runGenerateConfig to avoid false-positive symlink errors. Additionally, it suggests unifying role label validation logic using a strict regex pattern.

Comment thread scripts/ickb-live-preflight.mjs Outdated
Comment thread scripts/ickb-live-config-from-env.mjs
Comment thread scripts/ickb-generate-config.mjs Outdated
Comment thread scripts/ickb-live-preflight.mjs
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new script ickb-live-config-from-env.mjs to rebuild testnet configurations from environment variables without exposing private keys, along with corresponding tests. It also updates the config generator and preflight scripts to support optional RPC URLs and a new maxRetryableAttempts parameter, while refactoring path resolution to safely handle symlinked repository roots. The review feedback identifies two important issues: a potential sensitive data exposure where temporary files containing private keys may not be cleaned up if writing fails before they are staged, and a robustness issue in the rollback logic of replaceStagedConfigs where a failure during backup restoration can halt the entire rollback process and swallow the original error.

Comment thread scripts/ickb-live-config-from-env.mjs
Comment thread scripts/ickb-live-config-from-env.mjs
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new script ickb-live-config-from-env.mjs to rebuild testnet configurations from environment variables safely, and updates existing scripts (ickb-generate-config.mjs and ickb-live-preflight.mjs) to support an optional maxRetryableAttempts parameter and optional rpcUrl. It also adds role-specific CKB reserves and improves path validation across scripts. The review feedback suggests unifying role label validation with strict regex patterns, preserving original error contexts as the cause property, using conditional property spreading to avoid writing explicit undefined properties, and extracting duplicated path validation utilities into a shared package.

Comment thread scripts/ickb-live-preflight.mjs
Comment thread scripts/ickb-live-config-from-env.mjs
Comment thread scripts/ickb-live-config-from-env.mjs
Comment thread scripts/ickb-live-config-from-env.mjs
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new script, ickb-live-config-from-env.mjs, to rebuild testnet configurations from environment variables without exposing private keys. It also updates the config generator and preflight scripts to support optional RPC URLs (defaulting to CCC's endpoint when omitted) and a new maxRetryableAttempts option, alongside robust path validation through symlinked repository roots. The review feedback highlights an opportunity to extract duplicated filesystem utility functions into a shared helper file and suggests improving error handling in defaultCheckIgnored to gracefully handle cases where git is missing or fails to execute.

Comment thread scripts/ickb-live-config-from-env.mjs Outdated
Comment thread scripts/ickb-live-config-from-env.mjs Outdated
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new script, scripts/ickb-live-config-from-env.mjs, to rebuild testnet configurations from environment variables securely. It also updates scripts/ickb-generate-config.mjs and scripts/ickb-live-preflight.mjs to support optional RPC URLs, configure retryable attempts, and handle symlinked repository roots safely. Additionally, the preflight script now reports role-specific CKB reserves and capital requirements. The review feedback suggests preserving the original error context as the cause property when re-throwing errors, and extracting duplicated filesystem and path validation helpers into a shared utility file to reduce code duplication.

Comment thread scripts/ickb-live-config-from-env.mjs
Comment thread scripts/ickb-live-config-from-env.mjs
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new script live:config-from-env to generate testnet configurations from environment variables safely, and updates existing configuration generation and preflight scripts to support optional RPC URLs and a new maxRetryableAttempts parameter. Additionally, file writing operations are refactored to use atomic staging and symlink checks. Feedback suggests preserving the original error context when catching and re-throwing URL parsing errors in the new environment configuration script.

Comment thread scripts/ickb-live-config-from-env.mjs
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new script ickb-live-config-from-env.mjs to rebuild standard testnet configurations from environment variables without exposing private keys. It also updates the bot and tester configurations to make rpcUrl optional (allowing CCC to use its default endpoint) and adds support for an optional maxRetryableAttempts parameter. Additionally, the preflight script is updated to enforce role-specific CKB reserves (1,000 CKB for bots and 2,000 CKB for testers) and report spendable CKB separately. Path resolution logic across scripts has been refactored to safely handle symlinked repository roots, and file writing has been improved to use a staged temp-file approach to prevent partial writes. No review comments were provided, so there is no feedback to address.

@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

LGTM

Phroi %183

@phroi
phroi merged commit 4e0d927 into master May 27, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant