Skip to content

oor: add OOR_REJECT_USER_BALANCE typed reject code + classifier - #807

Merged
Roasbeef merged 2 commits into
mainfrom
oor-user-balance-reject-code
Jun 27, 2026
Merged

oor: add OOR_REJECT_USER_BALANCE typed reject code + classifier#807
Roasbeef merged 2 commits into
mainfrom
oor-user-balance-reject-code

Conversation

@Roasbeef

Copy link
Copy Markdown
Member

In this PR, we add a dedicated typed reject code for the operator's per-user
balance cap on the OOR path, plus the client-side classifier that routes it.

The arkd server rejects an OOR submit that would push a recipient mailbox
over its MaxUserBalance, but until now it borrowed OOR_REJECT_OUTPUT_POLICY
on the wire to avoid a proto change. That left a custodial sender (e.g.
swapdk) unable to tell a balance rejection from a per-VTXO policy rejection
without string-matching the reason, and the two want opposite recovery.

A per-VTXO policy rejection is permanent for a given output shape, so the
caller must restructure the outputs. A balance rejection is transient: it
clears once the recipient spends or refreshes its balance down, so a
custodian should retain the value and retry later rather than restructuring.

We add OOR_REJECT_USER_BALANCE to the OORRejectCode enum and teach
ClassifySubmitError to map it onto a new typed ErrUserBalanceExceeded,
documented as transient, so a wallet routes recovery on the code alone. A
classifier test pins the mapping and asserts the balance and output-policy
errors don't alias each other.

The arkd server side that emits this code (and the end-to-end itest) lives in
the darepo PR that bumps this submodule.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new client-facing typed error, ErrUserBalanceExceeded, which is returned when an OOR submit is rejected due to a recipient's aggregate VTXO balance exceeding the operator's limit. It maps the new OOR_REJECT_USER_BALANCE rejection code in ClassifySubmitError, updates the protobuf definitions, and adds comprehensive unit tests. Feedback on the tests suggests replacing require.NotErrorIs with require.False and errors.Is to ensure compatibility with older versions of the testify library.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread oor/errors_test.go
Comment on lines +46 to +47
require.NotErrorIs(t, balance, &ErrOutputPolicyViolation{})
require.NotErrorIs(t, policy, &ErrUserBalanceExceeded{})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The testify library does not standardly support NotErrorIs in many of its widely-used versions, which can lead to compilation failures depending on the project's testify dependency version. It is safer and more backward-compatible to use require.False with errors.Is instead.

Suggested change
require.NotErrorIs(t, balance, &ErrOutputPolicyViolation{})
require.NotErrorIs(t, policy, &ErrUserBalanceExceeded{})
require.False(t, errors.Is(balance, &ErrOutputPolicyViolation{}))
require.False(t, errors.Is(policy, &ErrUserBalanceExceeded{}))

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3e00b43677

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread oor/errors.go
Comment on lines +150 to +153
case oorpb.OORRejectCode_OOR_REJECT_USER_BALANCE:
return &ErrUserBalanceExceeded{
Reason: rejected.Reason,
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Carry the balance reject code to callers

When arkd returns OOR_REJECT_USER_BALANCE on the async SubmitPackage response, the only production consumer I found (darepod/server.go's registerOOREventRoutes) calls this classifier and immediately flattens the result to classified.Error() in OutboxErrorEvent.ErrorReason; GetOORSession/ListOORSessions then expose only that string. That means swapdk/custodial callers still cannot route on the new transient code with errors.As or the enum without string-matching the failure reason, which is the recovery split this code is meant to provide. Please persist/expose the reject code (or a typed failure cause) through the session status/daemon API instead of only constructing a local typed error here.

Useful? React with 👍 / 👎.

@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Jun 27, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 1m 30s —— View job


PR Review: oor: add OOR_REJECT_USER_BALANCE typed reject code + classifier

  • Read changed files (oor/errors.go, oor/errors_test.go, proto + generated)
  • Verify proto + generated code consistency
  • Assess Gemini NotErrorIs concern
  • Assess Codex P2 (carry reject code to callers)
  • Post review

Overall this is a clean, well-scoped change. The new ErrUserBalanceExceeded mirrors the existing ErrLineageTooLarge / ErrOutputPolicyViolation pattern exactly (struct + Error() + Is()), the enum value OOR_REJECT_USER_BALANCE = 3 is appended without renumbering, the generated oorwire.pb.go is consistent with the .proto, and the doc comments correctly capture the key distinction — transient balance rejection (hold + retry) vs terminal output-policy rejection (restructure). The test matrix is thorough, especially the explicit assertion that the two errors don't alias each other (which is the whole point of the split).

On the two bot comments

1. Gemini — NotErrorIsrequire.False(errors.Is(...)) (oor/errors_test.go:46-47)

I'd decline this suggestion. It's based on a premise that doesn't hold here:

  • go.mod pins github.com/stretchr/testify v1.11.1. require.NotErrorIs has existed since testify v1.7.0, so there's no compilation risk.
  • It's already used elsewhere in this repo — ledger/handlers_test.go:1114, vtxo/manager_admission_test.go:1059,1101.

require.NotErrorIs is the idiomatic, more readable form and matches existing convention. The rewrite would be a step backwards in clarity for no benefit.

2. Codex P2 — carry the balance reject code to callers

This one is a legitimate and accurate observation, and worth acknowledging even though I'd treat it as a follow-up rather than a blocker for this PR.

The classifier produces a typed error, but the only production consumer in this repo — darepod/server.go:2674-2684 (registerOOREventRoutes) — immediately flattens it:

classified := oor.ClassifySubmitError(rejected)
return &oor.DriveEventRequest{
    SessionID: oor.SessionID(sessionID),
    Event: &oor.OutboxErrorEvent{
        OutboxType:  submitOutbox,
        Retryable:   false,
        ErrorReason: classified.Error(),   // <- typed code collapses to a string
    },
}, nil

GetOORSession / ListOORSessions then surface only that failure_reason string. So a custodial caller (swapdk) consuming the daemon API still can't distinguish a transient balance rejection from a terminal output-policy one without string-matching — which is exactly the recovery split the typed code is meant to enable. The typed routing currently only works for an in-process Go caller of ClassifySubmitError, not for an RPC client of darepod.

That said:

  • The PR body is explicit that this is narrowly scoped to the wire code + classifier, with server emission and the e2e itest landing in the darepo submodule-bump PR.
  • Persisting/exposing the reject code through the session status / daemon API touches the proto-level session status surface and is a meaningfully larger change.

Recommendation: land this PR as-is, and open a follow-up to thread the typed cause (the OORRejectCode enum or a typed failure-cause field) through OutboxErrorEvent → session snapshot → GetOORSession/ListOORSessions, so the daemon API exposes the code rather than just failure_reason. Without that, the transient-vs-terminal benefit doesn't reach out-of-process custodial callers. Plan the follow-up →

Minor notes

  • oor/errors.go doc comments and oor/CLAUDE.md's "Multi-Tree Ancestry + Lineage Cap" section describe the classifier mapping for lineage/output-policy but won't mention the new balance code — worth a small doc touch-up in the package CLAUDE.md if you want the knowledge map to stay current (not required by this PR).
  • Test coverage is good. One tiny gap: there's no case pinning OOR_REJECT_UNSPECIFIED → pass-through (the default/zero branch falls through to returning the original *SubmitRejectedError). The existing "nil and untyped pass through" case covers non-rejection errors but not an unspecified-code rejection. Optional.

Nice work — the code itself is correct and the transient/terminal semantics are clearly documented.
oor-user-balance-reject-code

Roasbeef added 2 commits June 26, 2026 19:50
In this commit, we append OOR_REJECT_USER_BALANCE to the out-of-round
submit rejection enum. The new value gives the server a distinct wire
code for recipient aggregate-balance rejections without changing any
existing enum numbers.

The generated Go bindings are refreshed in the same commit so clients
can classify the new code without hand-editing generated files.
In this commit, we route OOR_REJECT_USER_BALANCE onto a dedicated
ErrUserBalanceExceeded sentinel. This lets wallets and custodial
senders distinguish a transient balance-limit rejection from a
terminal output-policy rejection without parsing the reason string.

The balance error documents that callers can retain value and retry
once the recipient spends or refreshes below the operator limit. The
classifier tests pin the mapping and assert that balance and
output-policy errors do not alias each other.
@Roasbeef
Roasbeef force-pushed the oor-user-balance-reject-code branch from 3e00b43 to 744b4df Compare June 27, 2026 04:13
@Roasbeef
Roasbeef merged commit e835ca7 into main Jun 27, 2026
18 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.

1 participant