feat: retrieve withdrawal proof #1295
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a public SQL action Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Client
participant DB as Kwil DB (SQL Action)
participant Bridge as Hoodi Bridge (list_wallet_rewards)
participant Validators as Validators/Signatures
Test->>DB: CALL hoodi_get_withdrawal_proof(wallet_address)
DB->>Bridge: hoodi_bridge.list_wallet_rewards(wallet_address, false) (confirmed only)
Bridge-->>DB: rows with withdrawal params (recipient, amount, block_hash, root, proofs, signatures)
DB-->>Test: RETURN TABLE rows (chain, chain_id, contract, created_at, recipient, amount, block_hash, root, proofs, signatures)
Note right of Test: Tests validate merkle proofs and 65-byte signatures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go:
- Around line 130-140: The fallback loop that converts proofsRaw to proofs can
panic when elements are not []byte; update the loop that iterates over proofsRaw
to perform a safe type assertion for each element (p.([]byte)) with an ok check
and fail the test with a descriptive message (e.g., using
require.True/require.Errorf) when any element is the wrong type, ensuring proofs
is only populated with valid []byte values.
🧹 Nitpick comments (1)
tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go (1)
269-272: Consider clarifying the test name.The test validates multiple users with one withdrawal each in the same epoch, not multiple withdrawals by a single user. The function comment (lines 270-271) clarifies this, but consider renaming to
TestHoodiGetWithdrawalProofMultipleUsersfor consistency with the actual scenario being tested.
📜 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
🧰 Additional context used
🧬 Code graph analysis (1)
tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go (2)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-85)deployments/infra/config/node/values.go (1)
Values(96-110)
🔇 Additional comments (6)
tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go (4)
19-25: LGTM!Constants are well-documented with clear references to the migration source file.
38-100: Good test setup and flow.The test correctly validates the end-to-end withdrawal proof retrieval flow. The setup properly syncs the migration-created instance, initializes the extension, and performs a complete deposit-withdraw-finalize cycle.
197-267: LGTM!Good negative test case validating that pending (unfinalized) epochs are correctly excluded from the withdrawal proof results. This ensures the
with_pending = falseparameter in the SQL action works as intended.
273-406: LGTM!The test implementation correctly validates:
- Multiple users can have withdrawals in the same epoch
- Each user's proof contains their specific withdrawal amount
- Per-user isolation is maintained when querying proofs
Good use of chained transfers with
prevPointto establish proper ordering.internal/migrations/erc20-bridge/004-withdrawal-proof-action.sql (1)
1-25: LGTM! Clean implementation of the withdrawal proof action.The action correctly:
- Exposes confirmed withdrawals only (
with_pending = false)- Maps internal field names (
param_*) to user-friendly column names- Uses a VIEW action for read-only access
- Is PUBLIC for user accessibility
One consideration: The
$wallet_addressparameter accepts any TEXT. The codebase implements address format validation in similar functions (e.g.,ob_get_participant_idvalidates the 0x-prefixed 42-character format). Iflist_wallet_rewardsdoesn't perform upstream validation, consider adding format validation to provide clearer error messages for malformed input.go.mod (1)
22-23: Ensure Kwil DB dependency versions are available in the upstream repository.Both dependencies use the same commit hash (
b8af58932e24) with matching timestamps (2026-01-07 15:41:36 UTC), ensuring consistency. The pseudo-version format is syntactically valid. However, verify that this commit exists in the upstreamgithub.com/trufnetwork/kwil-dbrepository and thatgo mod tidyorgo mod downloadsuccessfully resolves both versions in your Go environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/migrations/erc20-bridge/004-withdrawal-proof-action.sql (1)
4-16: Consider adding input validation for the wallet address parameter.The
$wallet_addressparameter accepts any TEXT value without validation. While the underlyinghoodi_bridge.list_wallet_rewardsprecompile likely handles this, consider adding explicit validation (e.g., Ethereum address format check) at the action level to provide clearer error messages and fail fast for invalid inputs.Example validation pattern
CREATE OR REPLACE ACTION hoodi_get_withdrawal_proof($wallet_address TEXT) PUBLIC VIEW RETURNS TABLE ( chain TEXT, chain_id TEXT, contract TEXT, created_at INT8, recipient TEXT, amount NUMERIC(78, 0), block_hash BYTEA, root BYTEA, proofs BYTEA[], signatures BYTEA[] ) { + -- Validate Ethereum address format (0x + 40 hex chars) + IF $wallet_address !~ '^0x[a-fA-F0-9]{40}$' { + RETURN error('invalid wallet address format'); + } + -- with_pending = false means only return confirmed epochs (ready for withdrawal) FOR $row IN hoodi_bridge.list_wallet_rewards($wallet_address, false) { RETURN $row.chain, $row.chain_id, $row.contract, $row.created_at, $row.param_recipient, $row.param_amount, $row.param_block_hash, $row.param_root, $row.param_proofs, $row.param_signatures; } };tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go (1)
43-73: Consider extracting the fake ERC20 address to a constant.The hardcoded address
"0x0000000000000000000000000000000000000001"is used multiple times across test functions (lines 52, 66, and also in other test functions). Extracting it to a package-level constant would improve maintainability and reduce duplication.Suggested refactor
Add to the constants section:
const ( // Real Hoodi chain and addresses (from migrations) MigrationHoodiChain = "hoodi" MigrationHoodiEscrow = "0x878d6aaeb6e746033f50b8dc268d54b4631554e7" MigrationHoodiAlias = "hoodi_bridge" // Alias from migration 000-extension.sql TestERC20Address = "0x0000000000000000000000000000000000000001" // Fake ERC20 for testing )
📜 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
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go (1)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-85)
⏰ 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: acceptance-test
- GitHub Check: lint
🔇 Additional comments (5)
internal/migrations/erc20-bridge/004-withdrawal-proof-action.sql (1)
17-22: LGTM! Clean implementation with correct filtering.The implementation correctly filters to confirmed epochs only (
with_pending = false) and properly maps all fields from the precompile result. The comment on Line 17 clearly documents this important behavior.tests/extensions/erc20/erc20_bridge_withdrawal_proof_test.go (4)
1-26: LGTM! Well-structured test setup.The build tag, imports, and exported constants are properly defined. The comments clearly document the purpose of each constant and their relationship to migration files.
112-221: LGTM! Comprehensive verification with robust type handling.The test thoroughly validates:
- The public action returns exactly 10 columns including signatures
- Defensive type assertions handle different array representations ([][]byte vs []any)
- All returned fields match expected values
- Merkle proof and signature structures are correctly validated (32-byte proofs, 65-byte signatures)
- Edge case of empty proofs for single-element Merkle trees is correctly documented
The extensive logging will be helpful for debugging failures.
223-293: LGTM! Important negative test case for pending epoch filtering.This test correctly validates that the
with_pending = falseparameter in the SQL action filters out unfinalized epochs. The comment on Line 270 clearly documents the intentional omission of epoch finalization, making the test intent explicit.
295-432: LGTM! Excellent multi-user test coverage.This test validates that:
- Multiple users can retrieve their individual withdrawal proofs
- Each user receives exactly their withdrawal amount
- The proof isolation works correctly (User A gets 50 tokens, User B gets 25 tokens)
The use of chained deposits with
prevPoint(Line 335, 345) correctly maintains event ordering. The comment on Lines 296-297 appropriately documents the known limitation about testing multiple epochs.
resolves: https://github.com/truflation/website/issues/3039
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.