Skip to content

Conversation

@0xMayoor
Copy link

Variable shadowing causes PaymentClose errors to get swallowed.
The := on line 363 creates a new err, but line 366 checks the old one.

MsgCloseGroup and MsgPauseGroup call OnGroupClosed directly without
going through AccountClose first, so payment is still open when this runs.

If PaymentClose fails, lease/bid show closed but payment stays open.

Fix: use = instead of := and return the error directly.

@0xMayoor 0xMayoor requested a review from a team as a code owner January 17, 2026 06:53
@coderabbitai
Copy link

coderabbitai bot commented Jan 17, 2026

Walkthrough

The OnGroupClosed function in the keeper module has been updated to propagate payment closing errors up the call stack instead of logging them and continuing execution. The error handling pattern changed from a nested conditional block that logged errors to a direct return statement using an existing error variable assignment.

Changes

Cohort / File(s) Summary
Error Handling Pattern Update
x/market/keeper/keeper.go
Modified OnGroupClosed to return errors from PaymentClose operation immediately rather than logging and swallowing them; changes error propagation behavior for payment closure failures

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A payment that closes, no longer is lost,
When errors arise, we don't pay the cost,
No silent whispers, no logged-away pain,
Just truth in the return—clear as the rain! 🌧️✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing error propagation for PaymentClose in the OnGroupClosed function.
Description check ✅ Passed The description clearly explains the variable shadowing bug, its impact, and the fix being applied to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Variable shadowing causes PaymentClose errors to get swallowed.
The := on line 363 creates a new err, but line 366 checks the old one.

MsgCloseGroup and MsgPauseGroup call OnGroupClosed directly without
going through AccountClose first - so payment is still open when this runs.

If PaymentClose fails, lease/bid show closed but payment stays open.

Fix: use = instead of := and return the error directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant