Skip to content

feat(ledger-suite-orchestrator): disable archiving for newly installed ledger suites - #11417

Merged
gregorydemay merged 2 commits into
masterfrom
ic_DEFI-2986_2_disable-archiving-for-new-ledger-suites
Sep 7, 2026
Merged

feat(ledger-suite-orchestrator): disable archiving for newly installed ledger suites#11417
gregorydemay merged 2 commits into
masterfrom
ic_DEFI-2986_2_disable-archiving-for-new-ledger-suites

Conversation

@gregorydemay

@gregorydemay gregorydemay commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Stacked on #11407. Addresses the review remark about the hardcoded archive options for new suites.

Why

  • A token added while archiving is disabled (DEFI-2986) would get a fresh ledger with trigger_threshold: 2_000 and start archiving once past it, reintroducing the double-mint commit point (DEFI-2967) for that token.

What

  • Raise the default trigger_threshold for newly installed ledger suites to 4_200_000_000, the value used to disable archiving on the ckBTC ledger (proposal 143757).

🤖 Generated with Claude Code

https://claude.ai/code/session_01S568wBREAARUA9GvGP5JMP

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused configuration change is correct and adequately tested.

Pull request overview

Disables archiving for newly installed ledger suites by raising the archive trigger threshold.

Changes:

  • Sets the default threshold to 4.2 billion blocks.
  • Adds a unit test verifying the new default.
File summaries
File Description
src/scheduler/mod.rs Updates the default archive threshold.
src/scheduler/tests.rs Tests the disabled-archiving configuration.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Base automatically changed from ic_DEFI-2986_disable-archiving-for-the-ckevm-ledger-suites to master September 2, 2026 09:44
…d ledger suites

A token added while archiving is disabled (DEFI-2986) would otherwise
get a fresh ledger with trigger_threshold 2_000 and start archiving
once past it, reintroducing the double-mint commit point (DEFI-2967)
for that token. Raise the default to 4_200_000_000, the same value as
the ckBTC ledger upgrade (proposal 143757). To be reverted once
DEFI-2967 lands.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S568wBREAARUA9GvGP5JMP
@gregorydemay
gregorydemay force-pushed the ic_DEFI-2986_2_disable-archiving-for-new-ledger-suites branch from 865aaa1 to fcbee2b Compare September 7, 2026 06:28
@gregorydemay gregorydemay changed the title feat(ledger-suite-orchestrator): DEFI-2986 disable archiving for newly installed ledger suites feat(ledger-suite-orchestrator): disable archiving for newly installed ledger suites Sep 7, 2026
@gregorydemay
gregorydemay marked this pull request as ready for review September 7, 2026 06:54
@gregorydemay
gregorydemay requested a review from a team as a code owner September 7, 2026 06:54
@github-actions github-actions Bot added the @defi label Sep 7, 2026
@zeropath-ai

zeropath-ai Bot commented Sep 7, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to 3c279e8.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► rs/ethereum/ledger-suite-orchestrator/src/scheduler/mod.rs
    + Introduce ARCHIVING_DISABLED_TRIGGER_THRESHOLD constant and use it to set icrc1_archive_options.trigger_threshold
► rs/ethereum/ledger-suite-orchestrator/src/scheduler/tests.rs
    + Add tests for icrc1_archive_options to verify archiving is disabled for new ledger suites
Enhancement ► rs/ethereum/ledger-suite-orchestrator/test_utils/src/pocket_ic/flow.rs
    + Adjust comments to reflect removal of hard-coded explanatory lines for trigger_threshold in test setup

@mbjorkqvist mbjorkqvist left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @gregorydemay!

A few related things we may still want to address:

  • The comment in the test_utils here still mention the production value of 2_000 for the trigger_threshold.
  • The docs here still assume archiving will be done - a sentence could be added saying that with the current configuration, archiving is (temporarily) disabled.
  • A new SNS ledger suite would still have archiving enable, as per the configuration here. This is perhaps worth a discussion regarding if archiving in general should be disabled for SNSs also (upgrade args are currently not supported for SNS ledger suite upgrades, so this is not so straight-forward).

…for new ledger suites

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ADwWgvm2kmqneFi1NVk4t1
@gregorydemay

Copy link
Copy Markdown
Contributor Author

🤖 Thanks @mbjorkqvist! Addressed in 3c279e8:

  1. Removed the stale comment in test_utils mentioning the old 2_000 production value.
  2. Added a remark to the README lifecycle section stating that archiving is temporarily disabled for newly created ledger suites and why.
  3. SNS ledger suites: agreed this is worth a discussion, but it is out of scope for this PR. The double-mint risk motivating this change is specific to the ckEVM minters, and the SNS init configuration is owned by the NNS team.

@gregorydemay
gregorydemay added this pull request to the merge queue Sep 7, 2026
Merged via the queue into master with commit 5422152 Sep 7, 2026
73 of 74 checks passed
@gregorydemay
gregorydemay deleted the ic_DEFI-2986_2_disable-archiving-for-new-ledger-suites branch September 7, 2026 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants