-
Notifications
You must be signed in to change notification settings - Fork 3
chore: fetch unclaimed withdrawal proof #1296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughReplaced a single-row RETURN with a RETURN NEXT loop in the withdrawal-proof SQL action to emit all confirmed epochs (newest first) filtered by unclaimed withdrawals. Added validator-signature insertion in two tests. Bumped two kwil-db pseudo-versions in Changes
Sequence Diagram(s)(Skipped — changes do not introduce new multi-component control flow requiring a sequence diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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. Comment |
Time Submission Status
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/migrations/erc20-bridge/004-withdrawal-proof-action.sql:
- Around line 4-5: The migration/query currently returns all confirmed epochs by
delegating to the precompile hoodi_bridge.list_wallet_rewards(), which is
untested for multi-epoch behavior and can produce unbounded result sets; update
the SQL in withdrawal-proof-action (and any calling function) to accept an
optional integer limit parameter (e.g., limit_count) and pass it into or apply
it after hoodi_bridge.list_wallet_rewards() to cap results, add or adjust tests
to simulate multiple confirmed epochs per wallet to verify ordering and
correctness, and document the new optional limit parameter in the function
signature and comments.
🧹 Nitpick comments (1)
tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go (1)
395-402: Test robustness: consider tolerating multi-row proofs (and add a consistent error message).
Given the action now returns all confirmed epochs, asserting exactly1proof row can become brittle if fixtures ever create multiple matching epochs. Consider asserting “>= 1” and validating at least one row matches the expected epoch (or explicitly validating ordering and taking the newest row). Also, add the same"failed to add validator signature"message for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/migrations/erc20-bridge/004-withdrawal-proof-action.sqltests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and Push
- GitHub Check: slow-integration-tests
- GitHub Check: lint
- GitHub Check: acceptance-test
🔇 Additional comments (2)
tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go (1)
101-108: Good: add validator signature before requesting withdrawal proofs.
This makes the signatures column meaningfully testable and prevents flakiness from “no signatures” proofs.go.mod (1)
22-23: go.sum correctly updated for kwil-db dependency bump.
Bothgithub.com/trufnetwork/kwil-dbandgithub.com/trufnetwork/kwil-db/coreare version-aligned at commit 1229f5f7af8c, and the correspondinggo.sumentries are present. The codebase imports (crypto, testing, erc20-bridge) resolve correctly, and tests for withdrawal functionality are in place.
resolves: resolves: https://github.com/truflation/website/issues/3041
Summary by CodeRabbit
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.