Skip to content

refactor: remove owner field from UpdateBalanceArgs#169

Open
lpahlavi wants to merge 5 commits intomainfrom
lpahlavi/update-balance-caller-check
Open

refactor: remove owner field from UpdateBalanceArgs#169
lpahlavi wants to merge 5 commits intomainfrom
lpahlavi/update-balance-caller-check

Conversation

@lpahlavi
Copy link
Copy Markdown
Contributor

@lpahlavi lpahlavi commented Apr 24, 2026

Summary

  • Removes the owner: Option<Principal> field from UpdateBalanceArgs, so update_balance always uses the caller's principal as the owner
  • Prevents callers from registering automated deposit monitoring on behalf of arbitrary principals
  • Updates integration tests and fixtures accordingly

🤖 Generated with Claude Code

The `update_balance` endpoint now always uses the caller's principal as
the owner, preventing callers from registering automated deposit
monitoring on behalf of arbitrary principals.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 24, 2026 07:24
Replace `default_update_balance_args()` fixture helper and explicit
`UpdateBalanceArgs { subaccount: None }` literals with
`UpdateBalanceArgs::default()`. Also updates the Candid interface to
remove the now-deleted `owner` field and its stale doc comment.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

The loop in `should_fail_for_anonymous_owner` covers endpoints that
accept an explicit `owner` field. Since `update_balance` no longer has
an `owner` field, calling it with `DEFAULT_CALLER` (case 1 of the loop)
would succeed, not reject. Move the `update_balance` assertion below the
loop where it is tested only with an anonymous caller, matching how
`withdraw` is already handled.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lpahlavi lpahlavi changed the title refactor: remove owner field from UpdateBalanceArgs refactor: remove owner field from UpdateBalanceArgs Apr 24, 2026
The accounts list included an entry with a different owner
(Principal::from_slice(&[1])). With the old API this registered that
principal's account. Now that update_balance always uses the caller,
that call is a duplicate of the DEFAULT_CALLER/no-subaccount entry and
emits no event, so the expected-count assertion failed (3 events vs 4).
Remove the entry; the test now only covers subaccount variants for the
caller.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 24, 2026 08:16
…nce test

Since update_balance always uses the caller as owner, the test no longer
needs a list of Account structs. Iterate over subaccounts directly and
derive the expected accounts by pairing them with DEFAULT_CALLER.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@lpahlavi lpahlavi requested review from THLO and maciejdfinity April 24, 2026 09:01
@lpahlavi lpahlavi marked this pull request as ready for review April 24, 2026 09:03
@lpahlavi lpahlavi requested a review from a team as a code owner April 24, 2026 09:03
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.

2 participants