chore: name the bridge a transfer charged its fee in - #1410
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPublic ERC20 bridge transfer actions now record their originating bridge in transaction-event metadata. The ledger test updates its expectation to validate the new bridge field for transfer events. ChangesBridge metadata attribution
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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
Submit or update total time with: Add time on top of previous submission with: 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.
🧹 Nitpick comments (1)
tests/streams/transaction_events_ledger_test.go (1)
277-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover all updated transfer actions.
This test exercises only
sepolia_transfer, so changes toethereum_transfer,hoodi_tt_transfer,hoodi_tt2_transfer,eth_truf_transfer, andeth_usdc_transfercould regress without detection. Add focused or parameterized cases asserting the expected bridge for each action.🤖 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 `@tests/streams/transaction_events_ledger_test.go` around lines 277 - 285, Extend the transfer-event metadata tests around the existing sepolia_transfer case to cover ethereum_transfer, hoodi_tt_transfer, hoodi_tt2_transfer, eth_truf_transfer, and eth_usdc_transfer. Add focused or table-driven cases that assert each action’s expected bridge via the existing meta.String("bridge") check, preserving the current sepolia_bridge assertion.
🤖 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 `@tests/streams/transaction_events_ledger_test.go`:
- Around line 277-285: Extend the transfer-event metadata tests around the
existing sepolia_transfer case to cover ethereum_transfer, hoodi_tt_transfer,
hoodi_tt2_transfer, eth_truf_transfer, and eth_usdc_transfer. Add focused or
table-driven cases that assert each action’s expected bridge via the existing
meta.String("bridge") check, preserving the current sepolia_bridge assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 26065d74-b73b-4277-9e33-0118a3f5e117
📒 Files selected for processing (3)
internal/migrations/erc20-bridge/002-public-transfer-actions.prod.sqlinternal/migrations/erc20-bridge/002-public-transfer-actions.sqltests/streams/transaction_events_ledger_test.go
|
@holdex pr submit-time 2h |
1 participant
A peer-to-peer transfer records which bridge it charged its fee in, so the fee ledger describes its own token instead of leaving a reader to infer it.
Why
transaction_eventscarries no token column, and a transfer is the one method whose fee is not charged in $TRUF: it charges in the bridge it moves.eth_truf_transfertakes 1 TRUF at 1e18,eth_usdc_transfertakes 1 USDC at 1e6, and both are booked under the samemethod_id = 4. The mainnet migration already documents this — "fee is paid in the SAME bridge as the transfer (not always in TRUF)" — but nothing recorded which one.A reader with only the ledger has to infer the token from the amount. That works while the two fees stay different constants and stops working the moment they do not, or a third bridge appears. One base unit differs between the two tokens by a factor of 1e12, so an inference that goes wrong is not a rounding matter.
What changes
record_transaction_eventalready takes ametadataargument, and every call site in the repo passesNULL. The transfer actions now pass the bridge instead:eth_truf_transfer→'{"bridge":"eth_truf"}'eth_usdc_transfer→'{"bridge":"eth_usdc"}'metadatais TEXT holding a JSON object —026-transaction-schemas.sqlnotes it would be JSONB if the engine supported it, and the ledger test helper already parses it as JSON — so the bridge is one key inside it rather than the whole value. It is empty on all 3,985,000+ existing mainnet rows, so nothing is displaced.The four dev transfer actions get the same treatment for parity. Note these reach no deployed network on their own:
scripts/migrate.shglobs./internal/migrations/*.sqlnon-recursively, and theinternal/migrationsembed is imported only by tests, soerc20-bridge/*.sqlis applied by hand exactly as the.prod.sqlheader describes.Test
TestTransactionEventsLedgerasserted a transfer carries no metadata, which is the behaviour being changed. It now asserts the bridge, so the stamp cannot be dropped silently.Scope
No schema change, no new method id, no new migration file, and no change to any fee amount or recipient. Rolling back is re-applying the previous revision of the action.
Worth knowing for review:
.prod.sqlis excluded from the//go:embedand fromgo test, so no automated check parses or executes those two literals. They were verified by reading them back out ofinfo.actionson the mainnet leader after the migration was applied.Related
Summary by CodeRabbit
Bug Fixes
Tests