fix(cketh): require two agreeing providers for the balance scan - #11243
Draft
gregorydemay wants to merge 2 commits into
Draft
fix(cketh): require two agreeing providers for the balance scan#11243gregorydemay wants to merge 2 commits into
gregorydemay wants to merge 2 commits into
Conversation
The ckERC20 automatic-deposit balance scan queried the latest block with a 3-of-4 client and reduced each batch with `AnyOf`, so a single provider answering a well-formed all-zeros `balanceOf` could decide the scan whenever the others disagreed or failed to decode — hiding a funded deposit address. Switch it to a dedicated 2-of-3 client reduced with `NoReduction`: two providers must agree, and a batch they disagree on is a chunk failure that is retried next tick rather than resolved from one provider's answer. A lower threshold than the minting path is acceptable here because the scan reads a non-finalized block only to notify the sweeper. Resolves the `TODO DEFI-2923` at the call site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates ckERC20 balance scanning to require agreement from two of three RPC providers.
Changes:
- Adds a dedicated 2-of-3 balance-scan RPC client.
- Uses
NoReductionfor disputed responses. - Updates unit and integration-test fixtures.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
test_utils/src/ckerc20.rs |
Stubs the configured provider count. |
eth_rpc_client/mod.rs |
Defines the dedicated threshold client. |
balance_scan/tests.rs |
Tests disagreement handling. |
balance_scan/mod.rs |
Uses the new client and reduction strategy. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| }; | ||
| impl Threshold { | ||
| /// The threshold [`balance_scan_rpc_client`] queries with. | ||
| pub const BALANCE_SCAN: Self = Self { total: 3, min: 2 }; |
Comment on lines
+118
to
+120
| /// than silently advanced until its next scheduled slot. A batch the providers disagree on is such | ||
| /// a failure: [`NoReduction`] never falls back to a single provider's answer, so one provider | ||
| /// claiming an empty balance cannot hide a funded address. |
The comment claimed the naming followed from `ConsensusStrategy::Threshold` requiring as many named services as it queries. That constraint is real but is a consequence, not the cause: `EthSepolia(None)` would work the same way mainnet does. The services are named because the EVM RPC canister's supported Sepolia set still holds `rpc.sepolia.org`, which this minter dropped, and naming is the only way to keep the canister from picking it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Resolves DEFI-2964, raised in review of #10873.
The ckERC20 automatic-deposit balance scan read the latest block through a 3-of-4 RPC client and reduced each batch with
AnyOf. Whenever the other providers disagreed or returned a decode failure, a single bad or malicious provider answering a well-formed all-zerosbalanceOfcould decide the scan on its own — the scan would see nothing and a funded deposit address would never be flagged.The scan now uses its own 2-of-3 client reduced with
NoReduction, so two providers must agree before the minter acts on a batch, and a batch they disagree on is treated as a failed chunk and retried on the next tick. A lower threshold than the minting path is acceptable here because the scan reads a non-finalized block height only to notify the sweeper; nothing is minted off this path. The finalized-deposit log-scraping client is unchanged.This resolves the
TODO DEFI-2923that already anticipated the change at the call site.Notable in the diff: the balance-scan threshold is exposed as
Threshold::BALANCE_SCANso the integration-test fixture derives how many providers to stub from production rather than restating the number — the mock asserts every stub is consumed, so a drift there fails the ckERC20 suite.