Skip to content

ENG-660: reject and disable payment splits to inactive destination accounts - #527

Merged
islandbitcoin merged 7 commits into
mainfrom
jabariennis/eng-660-split-destination-active
Oct 12, 2026
Merged

islandbitcoin merged 7 commits into
mainfrom
jabariennis/eng-660-split-destination-active

Conversation

@islandbitcoin

@islandbitcoin islandbitcoin commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Problem

TEST E2E for ENG-653 (#526) found that walletSplitsSet accepted a destination whose account's latest status was closed. Every later receive then failed at payout with InactiveAccountError and pushed the owner a "split failed" notice, with no end.

Change

  • Save time. setPaymentSplits loads each destination's account (walletId or username path) and rejects it with the new SplitDestinationInactiveError unless AccountValidator(account).isActive() passes. That is the same check the intraledger send applies to the recipient. The set stays all-or-nothing, so one inactive destination saves nothing. The error-map entry returns "Split destination account is not active".
  • Payout time. When a payout fails with InactiveAccountError, the executor looks up the destination account directly, because the error does not say which side was inactive. If the destination is inactive, the row is marked failed and the rule is turned off with disableForDestination(walletId, destinationWalletId), an updateMany on the wallet's enabled rules to that destination (enabled: true → enabled: false). It matches by destination, not by the execution's ruleId, because replaceForWallet re-inserts every rule with a new id on each save, so a recorded ruleId can name a rule that no longer exists. Only the call that flipped a flag pushes the owner, using the new notification.paymentSplit.disabled phrase (en + es) and the payment_split_disabled data type, so several queued rows for one rule produce one push. When nothing flipped, the executor calls existsForDestination: if no rule to that destination exists any more (edited away or deleted), or that lookup fails, the owner gets the ordinary failed push, so a failed payout is never silent. A source-account InactiveAccountError, or a destination lookup that fails, keeps the rule enabled and sends the usual failure push. If the disable write itself fails, the owner gets the usual failure push and an error is logged.
  • Record path. The webhook and the reconcile both record through recordSplitExecutions, which already lists rules with enabledOnly: true, and the reconcile uses listWalletIdsWithEnabledRules. No change was needed. Tests now pin both.

How it was tested

  • Unit tests:
    • set-splits.spec: closed, locked and pending destinations are rejected through both the walletId and username paths, with nothing saved. An active destination still saves. A mixed set with one inactive destination saves nothing. An unreadable destination account is treated as not found.
    • execute-pending-splits.spec: a closed, locked or pending destination disables the rule, fails the row and pushes the owner once. Two rows on one rule produce one push. A source-inactive error leaves the rule enabled. A failed destination lookup and a non-inactive error never disable the rule. A failed disable write falls back to the failure push. When the recorded rule no longer exists, the owner still gets the failure push. When nothing was disabled and the existsForDestination lookup fails, the owner gets the failure push.
    • record-split-executions.spec: a disabled rule records nothing.
    • wallet-splits-set.spec: the GraphQL error message for an inactive destination.
  • Integration test (payment-splits-repositories.spec, against a local docker mongo): disableForDestination returns true once, then false. An unknown destination returns false. After a re-save gives the rule a new id, the replacement rule is still disabled. The call is scoped to the wallet: another wallet's enabled rule to the same destination stays enabled (verified to fail if the walletId filter is removed). existsForDestination is true for a disabled rule and false for a destination never configured. A disabled rule records nothing, and a wallet whose only rule is disabled drops out of listWalletIdsWithEnabledRules.
  • With the executor change reverted, the new executor tests fail (5 failures).
  • Checks run: make unit-in-ci (293 suites, 3795 passed, 3 skipped), yarn test:integration (5 suites, 31 passed), yarn tsc-check-noimplicitany, yarn tsc-check, yarn eslint-check, yarn build, yarn check:sdl, yarn check-yaml, yarn madge-check, prettier on the changed files, and typos --config typos.toml. All pass.

Follow-ups

  • Out of scope per plan: no retroactive scan of existing rules. The feature is off in prod and TEST has none.
  • Mobile: payment_split_disabled is a new push type. The app should route it like payment_split_failed.
  • Disabled rules are still returned by Wallet.splits with enabled: false. The client should show them as off and let the owner delete or replace them.

🤖 Generated with Claude Code

https://claude.ai/code/session_01G4SaLJF5SWq6sjBEaWJFow

bobodread876 and others added 5 commits October 12, 2026 14:04
…ctive

walletSplitsSet now loads each destination's account and refuses it with
SplitDestinationInactiveError unless its current status is active, using the
same AccountValidator check the intraledger send applies to the recipient.
The set stays all-or-nothing: one inactive destination saves nothing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4SaLJF5SWq6sjBEaWJFow
…nactive

When a payout fails with InactiveAccountError and the destination account is
inactive right now (looked up directly, since the error does not say which
side failed), the executor fails the row, turns the rule off with a
conditional update, and pushes the owner a "split turned off" notice only
from the call that flipped it. A source-account InactiveAccountError keeps
the rule enabled and pushes the usual failure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4SaLJF5SWq6sjBEaWJFow
Covers the record path shared by the on-receive webhook and the reconcile
cron, and disableById against Mongo.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4SaLJF5SWq6sjBEaWJFow
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4SaLJF5SWq6sjBEaWJFow
The resolver fixture now answers the destination account lookup that
setPaymentSplits performs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4SaLJF5SWq6sjBEaWJFow
@linear

linear Bot commented Oct 12, 2026

Copy link
Copy Markdown

ENG-660

bobodread876 and others added 2 commits October 12, 2026 14:13
…n, never drop the push

A save re-inserts every split rule with a new id and a delete removes it,
so an execution's ruleId can name a rule that no longer exists. Disabling
by that id matched nothing and the owner got no push at all, while a
replaced rule to the same closed destination stayed enabled.

Disable by (walletId, destinationWalletId) instead, and when nothing was
disabled and no rule to that destination exists any more, send the
ordinary "failed" push.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4SaLJF5SWq6sjBEaWJFow
…llet

The scoping assertion ran against a wallet with no rules, so it passed
even with the walletId filter removed. Give wallet-g a real enabled rule
to the same destination, disable wallet-f's, and assert wallet-g's rule
is still enabled. Verified the test fails with the filter removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G4SaLJF5SWq6sjBEaWJFow
@islandbitcoin
islandbitcoin merged commit 374e5b9 into main Oct 12, 2026
15 checks passed
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