Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates go.mod dependencies for kwil-db modules and introduces four SQL history action views (hoodi_tt_get_history, hoodi_tt2_get_history, sepolia_get_history, ethereum_get_history) to expose unified ERC20 bridge transaction history with corresponding end-to-end test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
![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_history_test.go`:
- Line 34: The test currently discards errors from types.ParseDecimalExplicit
when creating transferAmt (and withdrawalAmt later), which can cause nil-pointer
panics; update the test to capture the returned error and assert it succeeded
using require.NoError(err) immediately after calling types.ParseDecimalExplicit
for transferAmt and withdrawalAmt so the test fails with a clear assertion
instead of panicking (keep the variables transferAmt/withdrawalAmt and the calls
to types.ParseDecimalExplicit unchanged, just add the require.NoError checks).
🧹 Nitpick comments (3)
internal/migrations/erc20-bridge/005-history-actions.sql (1)
1-86: Consider whether the pass-through wrapper pattern can be avoided.All four actions are identical except for the delegated source (
hoodi_tt,hoodi_tt2,sepolia_bridge,ethereum_bridge). If the SQL action language supports any form of dynamic dispatch or macro, that would eliminate maintaining four copies of the same column list and loop body. If not, this is fine as-is — just be aware that any schema change to the underlyingget_historyreturn type will require updating all four actions in lockstep.tests/extensions/erc20/erc20_bridge_history_test.go (2)
52-70: Magic column indices (0, 1, 6, 7) are fragile and hard to read.If the schema of
get_historyever changes column order or adds/removes a column, every index here silently shifts. Consider introducing named constants or a small helper that mapsrow.Valuesinto a struct, e.g.:type historyRecord struct { Type string Amount *types.Decimal // ... Status string BlockHeight int64 }This would make the assertions self-documenting and resilient to schema changes.
88-88:withdrawalRecipientduplicatesTestEscrowA.The hardcoded address
"0x1111111111111111111111111111111111111111"is the same asTestEscrowAdefined incommon_test.go. UsingTestEscrowAdirectly (or a distinct address) would make the intent clearer — is withdrawing to the escrow address itself intentional, or should a different external address be used?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/extensions/erc20/erc20_bridge_history_test.go`:
- Line 90: The test sets withdrawalRecipient to the same value as the escrow
address (hard-coded "0x111...1111" which equals TestEscrowA), so update the test
to use a distinct external address (e.g., TestUserB or another test constant) to
ensure the test exercises recipient-vs-escrow behavior; locate the declaration
of withdrawalRecipient in tests/extensions/erc20/erc20_bridge_history_test.go
and replace the hard-coded escrow value with the distinct test address constant
(referencing TestUserB or a similarly named constant from common_test.go).
🧹 Nitpick comments (2)
tests/extensions/erc20/erc20_bridge_history_test.go (2)
54-71: Magic column indices make assertions fragile and skip key fields.The test accesses row values by positional index (
[0],[1],[6],[7]) with no guard that the column schema matches expectations. If the view's column order changes, these assertions will silently check the wrong fields. Additionally, indices[2]–[5](from/to address, tx hashes) are never asserted, reducing confidence in the history records' correctness.Consider either:
- Extracting named constants for column positions, or
- Adding at least a column-count assertion (
require.Len(t, transferRow, expectedCols)) to catch schema drift early, along with basic assertions on the skipped fields.
42-51:get_historyparameters lack documentation — verify semantics.The call passes
(TestUserA, int64(10), int64(0)). From context this appears to be(address, limit, offset), but there's no comment clarifying the parameter contract. If the second param is actually a block-height filter rather than a limit, the test could be passing by coincidence (10 ≥ all block heights used). A brief inline comment would prevent misreads.
1 participant
resolves: https://github.com/truflation/website/issues/3309
Summary by CodeRabbit