-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sui): custom sui_executeTransactionBlock #3662
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a new client method, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as Sui API
participant P as parseRPC
C->>API: Invoke SuiCall with method "SuiExecuteTransactionBlock" and parameters
API-->>C: Return raw JSON response
C->>P: Call parseRPC(raw response)
P-->>C: Return parsed SuiTransactionBlockResponse
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3662 +/- ##
===========================================
- Coverage 64.61% 64.57% -0.05%
===========================================
Files 470 470
Lines 32857 32907 +50
===========================================
+ Hits 21232 21249 +17
- Misses 10659 10690 +31
- Partials 966 968 +2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zetaclient/chains/sui/client/client_live_test.go (1)
121-145
: Add negative scenario testing
This test only validates a successful response. Consider adding coverage for malformed JSON, missing fields, or error responses to ensure robust handling of all edge cases and improve test reliability.zetaclient/chains/sui/client/client.go (2)
159-198
: Improve test coverage forSuiExecuteTransactionBlock
Static analysis indicates untested lines (167-173, 175-178, 180-190, 192-195, 197). Consider adding tests for optional parameters (Options
,RequestType
), as well as error scenarios, to ensure comprehensive coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 167-173: zetaclient/chains/sui/client/client.go#L167-L173
Added lines #L167 - L173 were not covered by tests
[warning] 175-178: zetaclient/chains/sui/client/client.go#L175-L178
Added lines #L175 - L178 were not covered by tests
[warning] 180-190: zetaclient/chains/sui/client/client.go#L180-L190
Added lines #L180 - L190 were not covered by tests
[warning] 192-195: zetaclient/chains/sui/client/client.go#L192-L195
Added lines #L192 - L195 were not covered by tests
[warning] 197-197: zetaclient/chains/sui/client/client.go#L197
Added line #L197 was not covered by tests
222-247
: Validate error handling inparseRPC
Lines 239-240 and 243-244 handle unsuccessful parse attempts but remain untested. Expanding tests with malformed JSON or missingresult
fields would improve reliability and coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 239-240: zetaclient/chains/sui/client/client.go#L239-L240
Added lines #L239 - L240 were not covered by tests
[warning] 243-244: zetaclient/chains/sui/client/client.go#L243-L244
Added lines #L243 - L244 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
zetaclient/chains/sui/client/client.go
(3 hunks)zetaclient/chains/sui/client/client_live_test.go
(1 hunks)zetaclient/chains/sui/signer/signer.go
(1 hunks)zetaclient/chains/sui/signer/signer_tx.go
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/sui/signer/signer.go
zetaclient/chains/sui/client/client_live_test.go
zetaclient/chains/sui/client/client.go
zetaclient/chains/sui/signer/signer_tx.go
🪛 GitHub Check: codecov/patch
zetaclient/chains/sui/client/client.go
[warning] 167-173: zetaclient/chains/sui/client/client.go#L167-L173
Added lines #L167 - L173 were not covered by tests
[warning] 175-178: zetaclient/chains/sui/client/client.go#L175-L178
Added lines #L175 - L178 were not covered by tests
[warning] 180-190: zetaclient/chains/sui/client/client.go#L180-L190
Added lines #L180 - L190 were not covered by tests
[warning] 192-195: zetaclient/chains/sui/client/client.go#L192-L195
Added lines #L192 - L195 were not covered by tests
[warning] 197-197: zetaclient/chains/sui/client/client.go#L197
Added line #L197 was not covered by tests
[warning] 239-240: zetaclient/chains/sui/client/client.go#L239-L240
Added lines #L239 - L240 were not covered by tests
[warning] 243-244: zetaclient/chains/sui/client/client.go#L243-L244
Added lines #L243 - L244 were not covered by tests
🔇 Additional comments (3)
zetaclient/chains/sui/signer/signer.go (1)
88-88
: Enhance traceability with broadcast logs
Logging the broadcast transaction digest is beneficial for debugging and traceability. No concerns here.zetaclient/chains/sui/signer/signer_tx.go (1)
81-82
: Confirm removal ofRequestType
Previously, the request potentially includedRequestType
(e.g.,WaitForLocalExecution
). Please verify that removing it aligns with the desired behavior and does not alter the expected outcomes.zetaclient/chains/sui/client/client.go (1)
5-5
: Appropriate JSON import
The addition of"encoding/json"
is necessary for the new parsing logic. This change is straightforward and approved.
Makes SUI signer broadcast tx in a proper way without relying on
WaitForLocalExecution
.Closes #3620
Summary by CodeRabbit