Skip to content

fix(station): fund external canisters only from the station balance - #673

Open
aterga wants to merge 3 commits into
mainfrom
claude/report-triage-eyfusw
Open

aterga wants to merge 3 commits into
mainfrom
claude/report-triage-eyfusw

Conversation

@aterga

@aterga aterga commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

External canisters monitored by the station could inherit the station's global obtain-cycles strategy when they had no obtain options of their own. This PR stops that:

  • CycleManager::add_external_canister registers an external canister with explicit obtain options that never obtain cycles. canfund therefore never falls back to the global strategy for it, including when the global strategy changes later.
  • canister_monitor_start and canister_monitor_restart (monitors restored after an upgrade) both use the new registration.
  • canister_monitor_start now accepts only an empty or Disabled obtain strategy. Previously it rejected only MintFromNativeToken.
  • The station and upgrader still use the global strategy, so their own top-ups are unchanged.
  • The cycle manager now uses the test-safe crate::core::ic_cdk::api::print, so it can run in unit tests.

Behaviour change: external canisters can only be topped up from the station's own cycles balance. While the station itself is low on cycles, external canisters are not topped up. Any stored monitor with an explicit per-canister obtain strategy has that strategy ignored after the upgrade.

Testing

  • New unit tests:
    • canister_monitor_start_rejects_cycle_obtain_strategies
    • monitored_external_canisters_never_obtain_cycles: covers fresh and restored monitors, with the global strategy set before registration and changed after it
  • New integration test external_canisters_are_funded_only_from_station_balance:
    • Normal top-ups from the station's balance still happen.
    • A top-up the station can't afford doesn't touch the ICP account.
    • Both hold before and after a station upgrade.
  • Both unit tests and the integration test fail on the previous commit.
  • cargo test -p station: 411 passed. cargo fmt --check and cargo clippy are clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Rr5M1gYD2ZN6MC7Zs2UFxf

External canisters registered for cycle monitoring without their own
obtain-cycles options inherited the station's global obtain strategy.
Register them with explicit obtain options that never obtain cycles, both
when monitoring starts and when it is restored after an upgrade, and only
accept an empty or `Disabled` obtain strategy when starting to monitor an
external canister. The station and upgrader keep using the global strategy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rr5M1gYD2ZN6MC7Zs2UFxf
Copilot AI balanced review requested due to automatic review settings October 19, 2026 15:54

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.

🟡 Changes recommended

Regression coverage is needed to verify funding isolation while preserving station-balance top-ups across restoration and global-strategy changes.

1 open issue
What changed in this PR

This PR isolates external-canister funding from the station’s global cycle-obtaining strategy.

Changes:

  • Registers external monitors with explicit no-obtain options.
  • Uses this registration for new and restored monitors.
  • Rejects external obtain strategies other than empty or Disabled.
File Description
core/​station/​impl/​src/​services/​external_canister.rs Updates monitor registration, restoration, and strategy validation.
core/​station/​impl/​src/​services/​cycle_manager.rs Adds external registration with a no-obtain override.

Review effort: Balanced

Comment thread core/station/impl/src/services/external_canister.rs
Add unit tests for starting and restoring external canister monitors with
cycle obtain strategies, and an integration test that checks monitored
external canisters are topped up from the station's balance, including
after an upgrade, and never obtain cycles from the station's ICP account.

Use the test-safe print helper in the cycle manager so that it can be
exercised in unit tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rr5M1gYD2ZN6MC7Zs2UFxf
@aterga
aterga marked this pull request as ready for review October 21, 2026 08:45
@aterga
aterga requested a review from a team as a code owner October 21, 2026 08:45
@zeropath-ai

zeropath-ai Bot commented Oct 21, 2026

Copy link
Copy Markdown

✅ No security or compliance issues detected. Reviewed everything up to c519d22.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► core/station/impl/src/services/cycle_manager.rs
    Register external canisters with explicit obtain options (no obtain cycles) and add corresponding helper; add NoObtainCycles implementation
► core/station/impl/src/services/external_canister.rs
    Switch from add_canister to add_external_canister for monitoring external canisters
    Adjust canister_monitor_start flow to disallow certain obtain strategies and use external canister monitoring logic
► tests/integration/src/cycles_monitor_tests.rs
    Add utilities and tests for external canisters monitoring and funding behavior

This branch has not been deployed

No deployments
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.

4 participants