feat(minter): add deposit_sol and deposit_status endpoint skeletons by gregorydemay · Pull Request #208 · dfinity/cksol

gregorydemay · 2026-09-23T07:22:04Z

First PR of the stack implementing the balance-sweep deposit flow designed in #207. It adds the deposit_sol update endpoint and the deposit_status query with their Candid types, both returning a constant Queued status with a sweepable amount of zero, so that the interface can be reviewed and the following PRs each replace one stage of the constant with real behaviour: balance check, sweep, finalization, mint, and finally removal of process_deposit.

No cycles are charged, no RPC call is made and no state is changed. The anonymous principal is rejected as for the existing endpoints. Covered by unit tests and a PocketIC integration test.

🤖 Generated with Claude Code

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved review issues were identified.

Review effort: Lite
Findings: None

What changed in this PR

Adds skeleton deposit_sol and deposit_status endpoints for the balance-sweep deposit flow, returning a constant queued status without state changes or external calls.

Changes:

Added Candid and Rust deposit types and endpoints.
Wired the sweep module into the minter.
Added unit, integration, and anonymous-caller tests.

File	Description
`minter/src/main.rs`	Registers the new endpoints.
`minter/src/deposit/sweep/tests.rs`	Tests skeleton behavior.
`minter/src/deposit/sweep/mod.rs`	Implements constant queued responses.
`minter/src/deposit/mod.rs`	Registers the sweep module.
`minter/cksol_minter.did`	Defines the public Candid API.
`libs/types/src/lib.rs`	Adds shared endpoint types.
`integration_tests/tests/tests.rs`	Covers endpoint and authorization behavior.
`integration_tests/src/lib.rs`	Adds integration test client helpers.

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

Introduce the Candid interface and shared types of the manual sweep flow: `deposit_sol` queues the deposit address of an account for a sweep and `deposit_status` reports the latest deposit of an account. Both endpoints resolve the owner to the caller by default, reject the anonymous principal, and for now return a constant `Queued` status with nothing to sweep. Reading the balance, sweeping, and minting follow in later changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…wner Exercise `deposit_sol` and `deposit_status` with an anonymous caller that names a non-anonymous owner, and with a non-anonymous caller paying for another owner, so that the owner argument is proven to be wired through. Drop the unit test that only enshrined the placeholder status. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

The public status API must use deposit IDs so earlier deposits remain retrievable.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Make deposit_status address historical deposits by DepositId · New

…nd deposit_status `deposit_sol` now returns the identifier of the queued deposit and `deposit_status` takes that identifier, like `withdraw` and `withdrawal_status` exchange the burn index. The `Queued` status no longer echoes the account, and `deposit_status` no longer resolves an owner, so it accepts any caller and returns null for an unknown identifier. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

deposit_status must distinguish unknown IDs from the placeholder deposit; a rustdoc nit also remains.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Correct DepositSolArgs rustdoc to document only deposit_sol · New

Resolved since last review (1)

Make deposit_status address historical deposits by DepositId

…indices Follow the crate's convention for boundary identifiers instead of a newtype with a hand-written Candid implementation, and fix the stale documentation of `DepositSolArgs`, which no longer serves `deposit_status`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot

Copilot review overview

🔵 Needs a closer look

DepositSolId is always reused as 0, preventing unique identification of requests and historical statuses.

Review effort: Lite
Findings: None

Resolved since last review (1)

Correct DepositSolArgs rustdoc to document only deposit_sol

Previously missed (1)

In code that hasn't changed since last review

Allocate unique persisted deposit IDs

minter/src/deposit/sweep/mod.rs:10

Every successful call returns the same DepositSolId (0). The public contract defines this as a sequence number that identifies each sweep for its entire lifetime, so a second request cannot be distinguished from the first and historical statuses will collide once state is implemented. Allocate a persisted monotonically increasing id instead of reusing the placeholder.

This issue also appears on line 13 of the same file.

gregorydemay · 2026-09-23T07:43:45Z

+pub fn deposit_sol(_account: Account) -> Result<DepositSolId, DepositSolError> {
+    Ok(PLACEHOLDER_DEPOSIT_ID)
+}


Here I would already ensure that the resolved account is not anonymous. See assert_non_anonymous_account. Add an integration test for that scenario.

🤖 Done in 32ec08f: the sweep module's deposit_sol now asserts the resolved account is non-anonymous via a shared assert_non_anonymous_account helper (main.rs keeps the same trap and message for the other endpoints). Covered by a should_panic unit test on the sweep function and the anonymous-caller integration loop.

gregorydemay · 2026-09-23T07:45:36Z

+    }
+
+    #[tokio::test]
+    async fn should_default_owner_to_caller() {


Let's have a single table-driven test for the cases:

User 1 calls deposit_sol

User 2 calls deposit_sol for user 1

Anonymous principal calls deposit_sol for user 1

Il all 3 cases outcome should be the same

🤖 Done in 32ec08f: one table-driven test should_queue_deposit_for_non_anonymous_owner over (user 1 for self, with and without explicit owner), (user 2 for user 1) and (anonymous for user 1), all asserting the same Ok(id) and Queued status.

gregorydemay · 2026-09-23T07:47:10Z

+mod deposit_sol_tests {
+    use super::{DEPOSITOR_ACCOUNT, deposit_sol};
+
+    #[test]
+    fn should_queue_deposit() {
+        let result = deposit_sol(DEPOSITOR_ACCOUNT);
+
+        assert!(result.is_ok());
+    }
+}
+
+mod deposit_status_tests {
+    use super::{DEPOSITOR_ACCOUNT, DepositSolStatus, deposit_sol, deposit_status};
+
+    #[test]
+    fn should_report_queued_deposit_with_nothing_to_sweep() {
+        let deposit_id = deposit_sol(DEPOSITOR_ACCOUNT).expect("deposit_sol should queue a sweep");
+
+        let status = deposit_status(deposit_id);
+
+        assert_eq!(
+            status,
+            Some(DepositSolStatus::Queued {
+                sweepable_amount: 0
+            })
+        );
+    }
+}


merge those tests, status will always be checked hand-in-hand with outcome of deposit_sol

🤖 Done in 32ec08f: merged into a single should_queue_deposit_with_nothing_to_sweep that feeds the id from deposit_sol into deposit_status.

…he deposit_sol tests The sweep module now rejects an anonymous owner itself, through a check shared with the other endpoints, so `deposit_sol` only resolves the owner in the canister entry point. The integration tests cover, in one table, a user depositing for themselves, another user paying for them, and the anonymous principal paying for them; the unit tests check the queued status hand in hand with the outcome of `deposit_sol`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot

Copilot review overview

🔵 Needs a closer look

The endpoints reuse ID 0 and report queued status for unknown IDs, preventing reliable deposit identification.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

deposit_status returns queued for unknown deposit IDs

minter/src/deposit/sweep/mod.rs:19

deposit_status ignores its argument and returns a queued status for every value, so deposit_status(123) reports a real deposit even though the Candid contract says unknown IDs return null. This makes mistyped or stale IDs indistinguishable from queued deposits; at minimum, the skeleton should only recognize its placeholder ID, and the real implementation must look up the requested ID.

gregorydemay added this pull request to stack #209 September 23, 2026 07:22

Copilot AI lite review requested due to automatic review settings September 23, 2026 07:26

Copilot started reviewing on behalf of gregorydemay September 23, 2026 07:27 View session