feat: transfer TRUF token between TN users#1163
Conversation
Implements public SQL actions that allow users to transfer TRUF tokens directly on TN. resolves: trufnetwork/truf-network#1192
WalkthroughAdds public ERC-20 bridge transfer actions for Sepolia and Mainnet, embeds erc20-bridge SQL seeds in migrations, bumps kwil-db pseudo-versions in go.mod, and introduces kwiltest-tagged end-to-end tests exercising sepolia transfer, validation, and balances. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant PublicAction as Public Action (sepolia_transfer / mainnet_transfer)
participant Validator as Address/Amount Validator
participant Bridge as Bridge Extension
Client->>PublicAction: call *_transfer(to_address, amount)
PublicAction->>Validator: check_ethereum_address(to_address)\nvalidate amount > 0
Validator-->>PublicAction: valid / error
alt valid
PublicAction->>Bridge: transfer(to_address, amount::NUMERIC(78,0))
Bridge-->>PublicAction: result (ok / err)
PublicAction-->>Client: return result
else error
PublicAction-->>Client: return validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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). (2)
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: 0
🧹 Nitpick comments (4)
internal/migrations/erc20-bridge/002-public-transfer-actions.sql (2)
21-35: Consider deduplication of transfer logic.The
mainnet_transferfunction is identical tosepolia_transferexcept for the bridge name. Consider creating a reusable pattern or documenting why separate implementations are necessary.While the current implementation is correct, you could reduce duplication by creating a helper pattern. However, given that these are SQL actions and there are only two networks, the current approach is acceptable for clarity and maintainability.
7-9: check_ethereum_address is defined — no action required.
Definition found at internal/migrations/001-common-actions.sql:504 (CREATE OR REPLACE ACTION check_ethereum_address). Add a short comment or README pointer to that migration for maintainability.tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (1)
186-199: Consider adding transaction error handling.The helper function
callSepoliaTransfercorrectly propagates errors but doesn't distinguish between different error types (validation vs execution errors). Consider adding more specific error handling if needed for debugging.The current implementation is functional, but you might want to add logging or more specific error types for better debugging in production:
func callSepoliaTransfer(ctx context.Context, platform *kwilTesting.Platform, from, to, amount string) error { engineCtx := engCtx(ctx, platform, from, 1, false) res, err := platform.Engine.Call(engineCtx, platform.DB, "", "sepolia_transfer", []any{to, amount}, func(row *common.Row) error { return nil }) if err != nil { - return err + return fmt.Errorf("sepolia_transfer call failed: %w", err) } if res != nil && res.Error != nil { - return res.Error + return fmt.Errorf("sepolia_transfer execution error: %w", res.Error) } return nil }go.mod (1)
22-23: kwil-db entries are pseudo-versions — prefer tagged releases for production.go.mod (lines 22–23) currently pins pseudo-versions (v0.10.3-0.20250915124855-c60f28b113d1 and core v0.4.3-0.20250915124855-c60f28b113d1); pseudo-versions encode a specific commit + timestamp. (golang.org)
Commit c60f28b113d1 exists in the repo. (github.com) The repo’s nearest published tags are v0.10.2 and core/v0.4.2 — no v0.10.3 / core v0.4.3 tags found. (github.com)
Action: switch these to an official semver tag for production, or keep the pseudo-versions but add a PR note justifying why you must pin to these commits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(1 hunks)internal/migrations/erc20-bridge/002-public-transfer-actions.sql(1 hunks)internal/migrations/migration.go(2 hunks)tests/extensions/erc20/erc20_bridge_transfer_actions_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (2)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)tests/extensions/erc20/common_test.go (6)
TestChain(20-20)TestERC20(24-24)TestUserA(25-25)TestAmount2(28-28)TestUserB(26-26)TestAmount1(27-27)
⏰ 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). (2)
- GitHub Check: acceptance-test
- GitHub Check: lint
🔇 Additional comments (8)
internal/migrations/migration.go (2)
10-10: LGTM! Clean embedding of ERC20 bridge migrations.The addition of
erc20-bridge/*.sqlto the embed directive properly includes the new transfer action migrations.
45-54: LGTM! Consistent directory processing pattern.The ERC20 bridge directory processing follows the same pattern as the existing
test_onlydirectory processing, maintaining code consistency.internal/migrations/erc20-bridge/002-public-transfer-actions.sql (2)
12-14: Amount validation uses safe numeric conversion.Good use of
NUMERIC(78, 0)for amount handling - this provides sufficient precision for ERC-20 token amounts while avoiding floating-point issues. The positive amount check is correctly implemented.
4-18: Require caller authentication for sepolia_transfer.internal/migrations/erc20-bridge/002-public-transfer-actions.sql (sepolia_transfer): the action validates $to_address and $amount but does not check @caller before calling sepolia_bridge.transfer; I could not find sepolia_bridge.transfer's implementation in the repository to verify it enforces caller authentication.
- Action: either add an explicit caller/role check (e.g., verify LOWER(@caller) is authorized) before calling the bridge, or ensure/implement caller authentication inside sepolia_bridge.transfer.
tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (4)
19-62: Well-structured end-to-end transfer test.The test properly validates the complete transfer flow including initialization, funding, balance verification, and transfer execution. Good use of helper functions for improved readability.
89-96: Comprehensive validation of zero amount transfers.Good coverage of edge cases for amount validation, correctly testing zero amounts which should be rejected.
132-183: Excellent test coverage for multiple transfers.The test thoroughly validates a complex transfer scenario with multiple users and sequential transfers. The balance calculations and assertions are correct.
106-117: Prioritize insufficient-balance validation (issue #1608)The insufficient-balance test is commented out in tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (lines 106–116) and a repo search found no existing balance-validation checks. Prioritize issue #1608, implement the validation at the appropriate layer, and re-enable the test.
MicBun
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (5)
19-22: Fix doc comment to match the action name used.It refers to sepolia_balance(), but the code calls sepolia_wallet_balance(). Align to avoid confusion.
-// TestSepoliaTransferActions tests the new sepolia_transfer() and sepolia_balance() public actions. +// TestSepoliaTransferActions tests the new sepolia_transfer() and sepolia_wallet_balance() public actions.
82-82: Stabilize assertions on error messages; correct a misleading comment.
- Error text checks are brittle. Use a consistent substring that matches the SQL (e.g., “invalid ethereum address”), and avoid coupling to exact phrasing for amount validations.
- The “Negative amount (string parsing will fail at conversion)” comment is inaccurate; parsing succeeds and validation should reject amount <= 0.
Apply:
- require.Contains(t, err.Error(), "Invalid Ethereum address format") + require.Contains(t, err.Error(), "invalid ethereum address") @@ - require.Contains(t, err.Error(), "Invalid Ethereum address format") + require.Contains(t, err.Error(), "invalid ethereum address") @@ - require.Contains(t, err.Error(), "Transfer amount must be positive") + require.Contains(t, err.Error(), "must be positive") @@ - // Test Case 4: Negative amount (string parsing will fail at conversion) + // Test Case 4: Negative amount should be rejected by amount > 0 validation @@ - require.Contains(t, err.Error(), "invalid ethereum address") + require.Contains(t, err.Error(), "invalid ethereum address")Also applies to: 91-92, 101-101, 102-104, 149-149
78-80: Drop redundant “if err == nil { t.Log(...) }” blocks; rely on require.Error.The extra branches add noise. require.Error already provides clear failures.
- if err == nil { - t.Log("ERROR: Expected error for invalid address format but got none") - } + // Expect an error @@ - if err == nil { - t.Log("ERROR: Expected error for address without 0x prefix but got none") - } + // Expect an error @@ - if err == nil { - t.Log("ERROR: Expected error for zero amount but got none") - } + // Expect an error @@ - if err == nil { - t.Log("ERROR: Expected error for negative amount but got none") - } + // Expect an error @@ - if err == nil { - t.Log("ERROR: Expected error for insufficient balance (nil) but got none") - } + // Expect an error @@ - if err == nil { - t.Log("ERROR: Expected error for insufficient balance (partial) but got none") - } + // Expect an error @@ - if err == nil { - t.Log("ERROR: Expected error for invalid address in balance query but got none") - } + // Expect an errorAlso applies to: 87-89, 96-98, 105-107, 113-115, 135-137, 145-147
229-236: Guard against type-assert panics when reading DECIMAL.Be defensive in tests to get a useful error instead of a panic if the action returns NULL or a non-DECIMAL.
- balance = row.Values[0].(*types.Decimal).String() + dec, ok := row.Values[0].(*types.Decimal) + if !ok || dec == nil { + return fmt.Errorf("expected DECIMAL, got %T", row.Values[0]) + } + balance = dec.String()
251-256: Type-assert the escrow column safely.Avoid a panic if the schema changes; fail the test with a clear message.
- escrow = row.Values[1].(string) // escrow is the second column + s, ok := row.Values[1].(string) // escrow is the second column + if !ok { + return fmt.Errorf("expected escrow string at column 2, got %T", row.Values[1]) + } + escrow = s
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/extensions/erc20/erc20_bridge_transfer_actions_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (2)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)tests/extensions/erc20/common_test.go (6)
TestChain(20-20)TestERC20(24-24)TestUserA(25-25)TestAmount2(28-28)TestUserB(26-26)TestAmount1(27-27)
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: acceptance-test
🔇 Additional comments (1)
tests/extensions/erc20/erc20_bridge_transfer_actions_test.go (1)
23-62: Verify seeded actions and escrow column order
- sepolia_transfer found: internal/migrations/erc20-bridge/002-public-transfer-actions.sql (CREATE OR REPLACE ACTION sepolia_transfer).
- sepolia_wallet_balance not found under internal — tests call this; seed/action is required.
- get_erc20_bridge_info not found under internal — tests call this; seed it and ensure it returns the escrow address in column 2 (tests expect escrow at column 2).
2 participants
Implements public SQL actions that allow users to transfer TRUF tokens directly on TN. resolves: https://github.com/trufnetwork/truf-network/issues/1192
Summary by CodeRabbit