Merged
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds bridge-related client, transport, action, and type support for wallet balance, token withdrawal, and withdrawal-proof retrieval; implements Action methods and integration tests; updates interfaces and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Actions as Action
participant Transport
participant Service as TN_Service
Client->>Actions: GetWalletBalance(ctx, bridge, wallet)
Actions->>Actions: validate inputs, build actionName
Actions->>Transport: call(actionName, [wallet])
Transport->>Service: forward call
Service-->>Transport: result
Transport-->>Actions: result or error
Actions-->>Client: balance or error
Client->>Actions: Withdraw(ctx, bridge, amount, recipient)
Actions->>Actions: validate inputs, parse amount, build actionName
Actions->>Transport: execute(actionName, [[recipient, amount]])
Transport->>Service: forward execute
Service-->>Transport: txHash
Transport-->>Actions: txHash or error
Actions-->>Client: txHash or error
Client->>Actions: GetWithdrawalProof(ctx, input)
Actions->>Actions: validate inputs, build actionName
Actions->>Transport: call(actionName, [wallet])
Transport->>Service: forward call
Service-->>Transport: proofs payload
Transport-->>Actions: payload or error
Actions-->>Client: []WithdrawalProof or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/integration/bridge_actions_test.go (1)
53-66:Withdrawsubtest can never fail — both error and success branches are unconditionally accepted.The current logic:
if err != nil { t.Logf(...) // always passes } else { assert.NotEmpty(...) // only fails if hash is empty AND err is nil }A nil error with an empty hash is the only failure, which is an unlikely edge case. The test provides no meaningful regression coverage. Since the test expectation is for a non-existent bridge to behave consistently with the other subtests (node-level error or mempool acceptance), at least assert one specific outcome.
♻️ Suggested improvement
- hash, err := tnClient.Withdraw(bridgeID, amount, recipient) - - if err != nil { - t.Logf("Withdraw returned error (acceptable): %v", err) - } else { - assert.NotEmpty(t, hash, "should return a transaction hash if no error") - t.Logf("Withdraw returned hash (acceptable): %v", hash) - } + hash, err := tnClient.Withdraw(bridgeID, amount, recipient) + + // Either a node error OR a valid mempool tx hash is acceptable, but not both absent. + if err != nil { + t.Logf("Withdraw returned error (acceptable): %v", err) + } else { + require.NotEmpty(t, hash, "must return a non-empty transaction hash when no error") + t.Logf("Withdraw returned hash (acceptable): %v", hash) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/bridge_actions_test.go` around lines 53 - 66, The Withdraw subtest currently accepts both error and success paths unconditionally; update the assertions in the tnClient.Withdraw(bridgeID, amount, recipient) block so the test fails unless one clear outcome holds — either assert that err is non-nil (expecting a node-level error for the non_existent_bridge) or assert that err is nil and hash is non-empty (mempool acceptance). Locate the Withdraw subtest variables bridgeID/amount/recipient and the call tnClient.Withdraw and replace the current if/else logging with a single assertion that enforces (err != nil) || (err == nil && hash != ""), e.g., require one of those conditions to be true so the test provides meaningful coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/contractsapi/bridge_actions.go`:
- Around line 28-32: GetWalletBalance currently checks only the outer slice
(res.Values) but then reads res.Values[0][0] which can panic if the inner slice
is empty, and fmt.Sprint on a nil cell yields "<nil>" instead of "0". Update the
return path in GetWalletBalance to: verify len(res.Values[0]) > 0 before
accessing [0][0], and treat a nil cell (res.Values[0][0] == nil) as zero by
returning "0", nil; otherwise return the formatted cell value. Ensure you
reference res.Values in your checks and keep the same function signature for
GetWalletBalance.
In `@core/tnclient/client.go`:
- Around line 302-318: Update the public API to accept context.Context for
wallet and withdrawal operations: add ctx context.Context to the signatures of
GetWalletBalance and Withdraw in the IAction interface (core/types/stream.go)
and the Client interface (core/types/tsn_client.go), then update all
implementations and callers to pass the context (including
Client.GetWalletBalance and Client.Withdraw in core/tnclient/client.go, the
bridge actions implementation in core/contractsapi/bridge_actions.go, the
transport stub in core/tnclient/actions_transport.go, and related integration
tests in tests/integration/bridge_actions_test.go); ensure each file imports the
context package and that calls to underlying actions use the passed ctx instead
of context.Background(), and run/update tests to reflect the new signatures.
In `@core/types/bridge_types.go`:
- Around line 26-37: The WithdrawalProof struct is missing the BlockHeight field
described in the API; add a new field named BlockHeight (e.g., type uint64) with
the json tag "block_height" to the WithdrawalProof struct (near the existing
CreatedAt field) so callers can correlate withdrawals with on-chain state, and
ensure any (de)serialization and usages of WithdrawalProof handle the new field
(update any marshalling, constructors, or tests referencing WithdrawalProof as
needed).
In `@core/types/stream.go`:
- Around line 131-138: The GetWalletBalance and Withdraw method signatures in
the IAction interface lack the ctx context.Context parameter—add ctx as the
first parameter to GetWalletBalance( ctx context.Context, bridgeIdentifier
string, walletAddress string) (string, error) and Withdraw(ctx context.Context,
bridgeIdentifier string, amount string, recipient string) (string, error); then
propagate this change to the parallel Client interface in
core/types/tsn_client.go and update all implementations and callers (notably the
methods in core/contractsapi/bridge_actions.go, core/tnclient/client.go,
core/tnclient/actions_transport.go) to accept the ctx argument and stop using
context.Background() (pass the incoming ctx through), and update integration
tests to provide an appropriate context when calling these methods.
---
Duplicate comments:
In `@core/types/tsn_client.go`:
- Around line 52-57: The interface in core/types/tsn_client.go is missing
context.Context on two methods (GetWalletBalance and Withdraw) similar to
core/types/stream.go; update the signatures of GetWalletBalance(bridgeIdentifier
string, walletAddress string) (string, error) and Withdraw(bridgeIdentifier
string, amount string, recipient string) (string, error) to accept ctx
context.Context as the first parameter (i.e., GetWalletBalance(ctx
context.Context, ...) and Withdraw(ctx context.Context, ...)), keep
GetWithdrawalProof as-is, and then update all implementations and call sites of
these methods to pass the context through (adjust any functions referencing
GetWalletBalance or Withdraw to supply ctx).
---
Nitpick comments:
In `@tests/integration/bridge_actions_test.go`:
- Around line 53-66: The Withdraw subtest currently accepts both error and
success paths unconditionally; update the assertions in the
tnClient.Withdraw(bridgeID, amount, recipient) block so the test fails unless
one clear outcome holds — either assert that err is non-nil (expecting a
node-level error for the non_existent_bridge) or assert that err is nil and hash
is non-empty (mempool acceptance). Locate the Withdraw subtest variables
bridgeID/amount/recipient and the call tnClient.Withdraw and replace the current
if/else logging with a single assertion that enforces (err != nil) || (err ==
nil && hash != ""), e.g., require one of those conditions to be true so the test
provides meaningful coverage.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/types/bridge_types.go (1)
25-38:BlockHeighttype is inconsistent withBridgeHistory.BlockHeight.
BridgeHistory.BlockHeight(line 12) isint64, whileWithdrawalProof.BlockHeight(line 31) isuint64. Both represent Kwil block heights. The inconsistency requires explicit type conversions whenever code handles both structs and creates a confusing public API contract.
uint64is the more semantically correct choice for a monotonically increasing block number. Consider aligningBridgeHistory.BlockHeighttouint64as well for consistency across the bridge-related types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/types/bridge_types.go` around lines 25 - 38, BridgeHistory.BlockHeight is int64 while WithdrawalProof.BlockHeight is uint64 causing awkward conversions; change the BridgeHistory struct's BlockHeight field type from int64 to uint64 (i.e., update BridgeHistory.BlockHeight to uint64) so both bridge-related types use the same unsigned width, and then update any call sites that assumed int64 to use uint64 or perform explicit casts where necessary (search for BridgeHistory, WithdrawalProof usages to adjust conversions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/types/bridge_types.go`:
- Around line 25-38: BridgeHistory.BlockHeight is int64 while
WithdrawalProof.BlockHeight is uint64 causing awkward conversions; change the
BridgeHistory struct's BlockHeight field type from int64 to uint64 (i.e., update
BridgeHistory.BlockHeight to uint64) so both bridge-related types use the same
unsigned width, and then update any call sites that assumed int64 to use uint64
or perform explicit casts where necessary (search for BridgeHistory,
WithdrawalProof usages to adjust conversions).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves: https://github.com/truflation/website/issues/3362
Summary by CodeRabbit
New Features
Documentation
Tests