-
Notifications
You must be signed in to change notification settings - Fork 116
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
refactor: use protocol contracts V2 with TON deposits #3439
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 📝 WalkthroughWalkthroughThe pull request updates the changelog with new features such as support for an advanced abort workflow (including a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as E2ERunner
participant T as TON Chain
U->>R: Initiate TONDepositAndCall
R->>T: Submit TON depositAndCall transaction
T-->>R: Return transaction details (callData)
R->>R: Filter: Compare callData with cctx.RelayedMessage
R->>T: Wait for cctx status to be OutboundMined
sequenceDiagram
participant T as TON Node
participant O as Observer
participant V as Vote System
T->>O: Transmit inbound deposit transaction
O->>O: Execute extractInboundData(tx) → inboundData
O->>V: Call voteInbound(inboundData)
V-->>O: Return vote result
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 #3439 +/- ##
===========================================
- Coverage 64.69% 64.63% -0.07%
===========================================
Files 455 458 +3
Lines 31495 31839 +344
===========================================
+ Hits 20377 20579 +202
- Misses 10229 10355 +126
- Partials 889 905 +16
|
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 (1)
changelog.md (1)
16-16
: Validate the New Changelog Entry for TON Deposits
The new changelog entry for PR 3439 is clear and concise. Please verify that the description sufficiently conveys that this refactor introduces protocol contracts V2 specifically for TON deposits, ensuring consistency with the style of other entries.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
changelog.md
(1 hunks)e2e/runner/ton.go
(1 hunks)pkg/contracts/ton/gateway_msg.go
(2 hunks)zetaclient/chains/ton/observer/inbound.go
(3 hunks)zetaclient/chains/ton/observer/inbound_test.go
(3 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.
pkg/contracts/ton/gateway_msg.go
zetaclient/chains/ton/observer/inbound_test.go
e2e/runner/ton.go
zetaclient/chains/ton/observer/inbound.go
🪛 GitHub Check: codecov/patch
zetaclient/chains/ton/observer/inbound.go
[warning] 221-221: zetaclient/chains/ton/observer/inbound.go#L221
Added line #L221 was not covered by tests
[warning] 233-233: zetaclient/chains/ton/observer/inbound.go#L233
Added line #L233 was not covered by tests
[warning] 243-243: zetaclient/chains/ton/observer/inbound.go#L243
Added line #L243 was not covered by tests
🔇 Additional comments (9)
e2e/runner/ton.go (1)
118-118
: LGTM! Simplified filter logic.The filter condition has been simplified to directly compare encoded call data, improving code clarity and consistency with the
Memo()
method changes.pkg/contracts/ton/gateway_msg.go (2)
62-62
: LGTM! Simplified memo structure.Returning an empty byte slice simplifies the memo structure by removing unnecessary recipient bytes.
80-80
: LGTM! Consistent memo handling.The change to return only
CallData
aligns with the simplified memo structure and improves code clarity.zetaclient/chains/ton/observer/inbound.go (3)
181-187
: LGTM! Well-structured data encapsulation.The
inboundData
struct effectively encapsulates TON inbound deposit data, improving code organization and reducing parameter passing.
216-244
: Add test coverage for error paths.The error paths in the
extractInboundData
function are not covered by tests. Consider adding test cases for:
- Deposit parsing failure (line 221)
- DepositAndCall parsing failure (line 233)
- Unknown operation error (line 243)
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 221-221: zetaclient/chains/ton/observer/inbound.go#L221
Added line #L221 was not covered by tests
[warning] 233-233: zetaclient/chains/ton/observer/inbound.go#L233
Added line #L233 was not covered by tests
[warning] 243-243: zetaclient/chains/ton/observer/inbound.go#L243
Added line #L243 was not covered by tests
267-287
: LGTM! Enhanced protocol V2 integration.The changes effectively integrate protocol V2 features:
- Proper handling of cross-chain calls
- Safe confirmation mode
- Structured inbound data usage
zetaclient/chains/ton/observer/inbound_test.go (3)
165-167
: LGTM! Comprehensive test assertions.The test assertions correctly validate:
- Empty message for regular deposits
- Proper receiver address format
- Cross-chain call flag state
229-231
: LGTM! Accurate test validations.The test assertions correctly validate:
- Hex-encoded call data
- Proper receiver address format
- Cross-chain call flag state
436-437
: LGTM! Consistent test assertions.The test assertions maintain consistency with the new memo structure and address format requirements.
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.
Nice
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.
looks good to me
Description
Closes #2967
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests