fix(escrow): save closed payments before account hooks

[chalabi2]

Summary

• Add a regression test for the account-close hook path that can call back into PaymentClose.
• Save dirty payments before the account during escrow batch saves.
• Prevent hooks from seeing a closed account while related payments are still open in the store.

Root cause

AccountClose marked related payments closed in memory, then save persisted the account before payments. Persisting the account fired the account-closed hook. The market hook could then call PaymentClose while the account was already stored as closed and the payment was still stored as open, producing account closed.

Verification

Failing-before output from the regression test:

=== RUN   Test_AccountClose_ClosesPaymentBeforeAccountHook
    keeper_test.go:338:
        Error:       Received unexpected error:
                     account closed
--- FAIL: Test_AccountClose_ClosesPaymentBeforeAccountHook

Passing-after checks:

GOWORK=off go test ./x/escrow/keeper -run Test_AccountClose_ClosesPaymentBeforeAccountHook -count=1 -v
GOWORK=off go test ./x/escrow/keeper ./x/market/keeper ./x/market/handler -count=1
git diff --check

[coderabbitai]

Walkthrough

This PR reformats a conditional block in the escrow keeper's save method without altering its logic, and adds a new test verifying that payments can be closed from within an account-closed hook before the account itself finishes closing.

Changes

Escrow Save Formatting and Hook Ordering Test

Layer / File(s)	Summary
Reformat dirty-account save block x/escrow/keeper/keeper.go	Whitespace/layout changes to the if acc.dirty block in save, preserving existing saveAccount call and error return behavior.
Hook-triggered payment closure test x/escrow/keeper/keeper_test.go	Adds Test_AccountClose_ClosesPaymentBeforeAccountHook, which registers an AddOnAccountClosedHook closing a payment, sets up bank mocks, calls AccountClose, and asserts no error.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

A hop, a skip, a tidy brace,
Formatting cleaned in its place.
A hook now checks before it's done,
Payments closed before accounts are none.
🐇 Thump thump — tests all pass, the race is won!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the escrow fix to persist closed payments before account hooks.
Description check	✅ Passed	The description matches the changeset and explains the regression test, root cause, and fix clearly.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/escrow-payment-close-order

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

x/escrow/keeper/keeper_test.go (1)

303-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a short comment documenting why the hook's PaymentClose branch is expected to be skipped post-fix.

As written, the interesting regression path (hook calling PaymentClose) is exercised only in the pre-fix/broken code; with the fix applied, the condition at line 311 is expected to be false because AccountClose already closed and persisted the payment before the hook fires. A one-line comment clarifying this would help future maintainers understand what's actually being asserted here, since the branch not firing is itself the proof of the fix.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@x/escrow/keeper/keeper_test.go` around lines 303 - 316, Add a short inline
test comment in the AddOnAccountClosedHook callback near the PaymentClose branch
to explain that this path should no longer run after the fix. Reference the
nested hook logic around GetPayment and PaymentClose so it’s clear that
AccountClose has already closed and persisted the payment before this hook
executes, making the StateClosed check expected to fail and the branch skipped.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@x/escrow/keeper/keeper_test.go`:
- Around line 303-316: Add a short inline test comment in the
AddOnAccountClosedHook callback near the PaymentClose branch to explain that
this path should no longer run after the fix. Reference the nested hook logic
around GetPayment and PaymentClose so it’s clear that AccountClose has already
closed and persisted the payment before this hook executes, making the
StateClosed check expected to fail and the branch skipped.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 05282eba-eac6-4e97-a0a3-2b050c67fc6b

📥 Commits

Reviewing files that changed from the base of the PR and between 7e903e6 and fad6150.

📒 Files selected for processing (2)

• x/escrow/keeper/keeper.go
• x/escrow/keeper/keeper_test.go