fix(ton): fix call data parsing (v29)#3714
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 updates the changelog with new version entries documenting various fixes and feature additions, updates dependency versions in the module file, and introduces several modifications and new test cases. Changes span across multiple components such as TON contract tests, gas limit handling in EVM chains, observer methods (for inbound/outbound voting), and interface/mocking enhancements for cross-chain transaction contexts and ballot fetching. Changes
Sequence Diagram(s)sequenceDiagram
participant O as Observer
participant C as CrossChainTx Service
participant B as Ballot Service
O->>C: GetCctxByHash(cctxIndex)
alt Context Found
O->>B: GetBallotByID(cctx identifier)
alt Ballot Not Found
B-->>O: Error (NotFound)
O->>O: Log warning "CCTX exists but ballot not found"
O-->>O: Return cctxIndex without posting vote
else Ballot Found
B-->>O: Return ballot data
O->>O: Post vote on inbound transaction
end
else Context Not Found
C-->>O: Not found error
O->>O: Continue alternative handling
end
Possibly related PRs
Suggested labels
🪧 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
|
![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 (1)
zetaclient/chains/base/observer.go (1)
406-423: Implement optimized ballot existence check before voting.This enhancement adds a critical optimization to the voting process by first checking if a cross-chain transaction context (CCTX) already exists, and if it does, verifying if its corresponding ballot exists. This prevents unnecessary vote operations when a CCTX exists but the ballot doesn't, which is a valid scenario in the finalization flow.
The implementation correctly handles the gRPC status code for "not found" cases, and includes helpful logging for debugging purposes.
Consider adding a comment explaining why we specifically check for the NotFound code and not other possible error conditions during the ballot retrieval.
+ // We only want to handle the specific case where the ballot is not found + // Other errors should flow through to the normal error handling path if st, ok := status.FromError(ballotErr); ok && st.Code() == codes.NotFound {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
changelog.md(1 hunks)go.mod(3 hunks)pkg/contracts/ton/gateway_test.go(2 hunks)pkg/contracts/ton/testdata/07-deposit-and-call-abi.json(1 hunks)pkg/contracts/ton/tlb.go(1 hunks)x/observer/types/chain_params.go(1 hunks)x/observer/types/chain_params_test.go(1 hunks)zetaclient/chains/base/observer.go(2 hunks)zetaclient/chains/base/observer_test.go(4 hunks)zetaclient/chains/evm/common/constant.go(0 hunks)zetaclient/chains/evm/observer/inbound_test.go(5 hunks)zetaclient/chains/evm/signer/gas.go(2 hunks)zetaclient/chains/evm/signer/gas_test.go(1 hunks)zetaclient/chains/evm/signer/outbound_data_test.go(1 hunks)zetaclient/chains/evm/signer/sign.go(0 hunks)zetaclient/chains/interfaces/interfaces.go(2 hunks)zetaclient/chains/sui/observer/observer_test.go(3 hunks)zetaclient/chains/sui/sui.go(0 hunks)zetaclient/chains/ton/liteapi/client.go(1 hunks)zetaclient/chains/ton/observer/inbound.go(1 hunks)zetaclient/chains/ton/observer/inbound_test.go(4 hunks)zetaclient/chains/ton/observer/observer.go(4 hunks)zetaclient/chains/ton/observer/observer_test.go(2 hunks)zetaclient/chains/ton/observer/outbound.go(4 hunks)zetaclient/testutils/mocks/zetacore_client.go(2 hunks)zetaclient/testutils/mocks/zetacore_client_opts.go(1 hunks)
💤 Files with no reviewable changes (3)
- zetaclient/chains/evm/common/constant.go
- zetaclient/chains/evm/signer/sign.go
- zetaclient/chains/sui/sui.go
🧰 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/ton/observer/inbound.gozetaclient/chains/evm/signer/outbound_data_test.gozetaclient/chains/ton/observer/inbound_test.gozetaclient/chains/ton/observer/observer_test.gozetaclient/chains/sui/observer/observer_test.gopkg/contracts/ton/tlb.gox/observer/types/chain_params.gozetaclient/chains/evm/signer/gas_test.gozetaclient/chains/interfaces/interfaces.gozetaclient/chains/ton/observer/observer.gopkg/contracts/ton/gateway_test.gozetaclient/chains/evm/observer/inbound_test.gozetaclient/chains/base/observer_test.gozetaclient/chains/evm/signer/gas.gox/observer/types/chain_params_test.gozetaclient/chains/ton/liteapi/client.gozetaclient/chains/base/observer.gozetaclient/chains/ton/observer/outbound.gozetaclient/testutils/mocks/zetacore_client_opts.gozetaclient/testutils/mocks/zetacore_client.go
🔇 Additional comments (53)
x/observer/types/chain_params_test.go (1)
219-223: Well-structured test case for DisableTssBlockScan field.This test case appropriately verifies that changes to the
DisableTssBlockScanfield affect the equality check between ChainParams instances. The test follows the established pattern of the other test cases in this function, maintaining consistency in the test suite.pkg/contracts/ton/testdata/07-deposit-and-call-abi.json (1)
1-8: New TON test data with appropriate structure and content.The JSON file is well-formed and contains all necessary fields for TON transaction testing. The presence of the
test: trueflag properly indicates this is test data. The description field provides clear context about the test case purpose (depositing 0.1 TON with abi-encoded payload).x/observer/types/chain_params.go (1)
469-470: Proper inclusion of DisableTssBlockScan in equality check.The modification correctly adds the
DisableTssBlockScanfield to the chain parameters equality check function. This ensures that differences in this field will be detected when comparing ChainParams instances, which is consistent with how other fields are handled in this function.go.mod (3)
18-18: Updated Cosmos SDK dependency to newer version.The Cosmos SDK dependency has been updated from v0.50.12 to v0.50.13, which appears to be a minor version update. According to the AI summary, this is part of a fix documented in the changelog (v29.0.3).
134-134: Converted fsnotify from indirect to direct dependency.The
// indirectcomment has been removed from the fsnotify dependency, indicating it's now being used directly in the codebase rather than as a transitive dependency.
313-313:Details
✅ Verification successful
Significantly updated tongo dependency version.
The tongo dependency has been updated from v1.9.3 to v1.14.8, which is a substantial version jump. This may include important new features or fixes for TON blockchain support.
🏁 Script executed:
#!/bin/bash # Check for breaking changes in the tongo library between versions git clone https://github.com/tonkeeper/tongo.git /tmp/tongo cd /tmp/tongo git log --oneline v1.9.3..v1.14.8 | grep -i "breaking" || echo "No explicit breaking changes found in commit messages" # Check if this version is being used in the codebase cd - rg -A 1 "github.com/tonkeeper/tongo" --type goLength of output: 7791
tongo v1.14.8 Dependency Update Verified
- The dependency in
go.modhas been updated from v1.9.3 to v1.14.8.- A check of the commit history between these versions did not reveal any explicit breaking changes.
- A repository-wide search confirms that the new version is referenced consistently across multiple modules, indicating that the update is integrated as expected.
- While automated checks are encouraging, it remains advisable to perform integration tests for TON blockchain functionalities to ensure complete stability.
pkg/contracts/ton/tlb.go (2)
35-38: Improved bit string handling for more efficient data extraction.The refactoring of
UnmarshalSnakeCellnow directly uses bit string operations to determine available bytes and extract data, avoiding unnecessary byte array allocation and trimming. This is a cleaner approach that should resolve the TON call_data parsing issue.
43-49: More efficient bit string construction for data encoding.The
MarshalSnakeCellimplementation has been streamlined to directly allocate a bit string of the appropriate size and write bytes to it, rather than going through an intermediatetlb.Bytesstructure. This reduces allocations and simplifies the code path.changelog.md (1)
3-28: LGTM - Well-structured changelog entries for new versions.The changelog entries clearly document the recent fixes and features, following the established format with proper PR references and categorization.
pkg/contracts/ton/gateway_test.go (2)
165-199: Comprehensive test coverage for ABI-encoded call data.This test case specifically validates the parsing of ABI-encoded call data in deposit-and-call transactions, which directly tests the fixes made to the snake cell encoding/decoding. The test includes appropriate assertions for all transaction properties and provides a clear example of ABI-encoded data handling.
401-446: Enhanced snake data testing with structured test cases.The refactored
TestSnakeDatafunction now:
- Includes a useful helper function for hex-to-byte conversion
- Provides a more comprehensive set of test cases including simple strings, long text, and ABI-encoded data
- Contains improved error reporting for length mismatches
These improvements ensure the snake cell marshaling/unmarshaling works correctly for various data types and sizes, particularly for complex ABI-encoded call data.
zetaclient/chains/ton/observer/inbound.go (1)
257-257: Improved gas limit handlingReplacing the hardcoded
0withmaxGasLimitis a good change that aligns with the broader refactoring of gas limit handling across the codebase. This change prevents potential transaction failures that might occur with zero gas limit while also making the code more maintainable.zetaclient/chains/sui/observer/observer_test.go (2)
116-116: Enhanced error handling test coverageGood addition of the mock for the "not found" error case. Testing how the system handles missing cross-chain transaction contexts is essential for ensuring robustness in production environments.
192-192: Consistent error handling testing approachAppropriately applying the same error case testing to the ProcessInboundTrackers test. This consistency ensures comprehensive coverage of error handling paths.
zetaclient/chains/evm/signer/outbound_data_test.go (1)
50-50: Improved gas limit assertionThis change correctly updates the test to verify that the outbound data uses the gas limit from the transaction context's outbound parameters, which aligns with the overall shift toward more dynamic gas limit handling in the codebase.
zetaclient/chains/ton/observer/observer_test.go (1)
179-182: Well-structured test helper methodGood addition of the
MockGetCctxByHashhelper method, which encapsulates the mock setup for testing "not found" error cases. This abstraction follows the DRY principle, making tests more readable and maintainable while ensuring consistent error handling tests across the codebase.zetaclient/chains/ton/observer/inbound_test.go (4)
144-144: Added mock for cross-chain transaction context.The addition of
ts.MockGetCctxByHash()after mocking the block header ensures that tests properly simulate the retrieval of cross-chain transaction contexts, which is essential for thorough testing of the inbound transaction handling logic.
209-209: Added consistent cross-chain transaction context mocking.This adds the same
MockGetCctxByHash()call as in the previous test case, maintaining consistency across different test scenarios and ensuring proper simulation of the cross-chain context retrieval for the deposit-and-call transaction type.
352-352: Added cross-chain context mocking for multiple transaction test.The addition of
ts.MockGetCctxByHash()after mocking block headers for all transactions ensures proper simulation of cross-chain transaction context retrieval in the multiple transactions test case.
420-420: Added consistent mocking in the inbound tracker test.The addition of
ts.MockGetCctxByHash()in theTestInboundTrackerfunction ensures that the test properly simulates the cross-chain transaction context retrieval, maintaining testing consistency with the other test functions.zetaclient/chains/evm/observer/inbound_test.go (4)
449-452: Added ZetacoreClient mock capability to test struct.The addition of the
mockZetacoreClientfunction field to the test struct enhances the test capabilities, allowing for comprehensive testing of ZetacoreClient interactions, particularly the cross-chain transaction context retrieval.
462-464: Implemented mock for CCTX not found scenario.The implementation sets up a mock for the
GetCctxByHashmethod to return a "not found" error, correctly simulating the scenario where a cross-chain transaction context cannot be found.
476-477: Set ZetacoreClient mock to nil for irrelevant test cases.This change explicitly sets the
mockZetacoreClientto nil for test cases that don't need to test ZetacoreClient interactions, ensuring clear intent and preventing unexpected mock behaviors.Also applies to: 487-488
500-502: Added conditional ZetacoreClient mock configuration.This implementation ensures that the ZetacoreClient mock is only configured when the test case provides a mock function, following a clean pattern that's consistent with the rest of the test suite.
zetaclient/chains/ton/liteapi/client.go (2)
52-55: Enhanced client connection configuration.This change improves the client connection configuration by:
- Setting the maximum number of connections based on the available lite servers
- Configuring a fixed number of workers per connection
These optimizations should improve request throughput and reliability when working with TON lite servers.
47-50:Details
❓ Verification inconclusive
Added performance optimization comment and constant for worker count.
The comment explains the rationale for configuring multiple connections per lite server, and the constant defines a fixed number of workers per connection for performance optimization.
Verify if 8 workers per connection is an optimal value based on typical TON lite server performance characteristics:
🏁 Script executed:
#!/bin/bash # Check if there are any performance-related discussions or benchmarks in the codebase rg -A 2 -B 2 "workers per connection|performance benchmark|connection scaling" --type goLength of output: 90
Performance worker count value requires manual verification.
The code update introduces the constantworkers = 8along with a comment explaining that multiple connections per lite server are used to enhance request performance. However, our search did not yield any internal benchmarks or discussions that confirm 8 is optimal based on typical TON lite server performance characteristics. Please review or conduct manual performance tests and, if available, add supporting documentation to validate this setting.zetaclient/chains/interfaces/interfaces.go (2)
94-94: Interface enhanced with method for retrieving CCTX by hash.The addition of
GetCctxByHashprovides functionality to retrieve cross-chain transactions by their hash identifiers, complementing the existingGetCctxByNoncemethod. This is a logical extension that improves the interface's versatility.
108-108: Interface expanded with ballot retrieval functionality.Adding
GetBallotByIDenhances the interface by providing a method to query ballots by identifiers, which aligns with the system's architecture around ballot tracking. This extension complements the existing interface methods for data retrieval.zetaclient/chains/evm/signer/gas.go (2)
13-14: Increased gas limit cap with improved documentation.The
maxGasLimithas been increased from 1,000,000 to 2,500,000 with a clear comment explaining its purpose as a preventive measure against excessive gas consumption. This change appropriately accommodates more complex EVM chain operations.
70-76: Simplified gas limit handling logic.The gas handling logic has been streamlined by removing the minimum gas limit check, keeping only the maximum limit enforcement. This simplification reduces code complexity while maintaining the essential guard against excessive gas usage.
zetaclient/chains/evm/signer/gas_test.go (2)
32-33: Updated test case to use standardized gas limit.The test has been refactored to use a fixed gas limit value of 21,000 instead of relying on the removed
minGasLimitvariable, which aligns with the simplified gas handling logic in the production code.
44-45: Standardized gas limit in London gas logic test.Gas limit has been standardized to 21,000 for consistency with other test cases, improving test maintainability and aligning with the removal of the minimum gas limit check in the implementation.
zetaclient/chains/ton/observer/observer.go (6)
5-5: Added atomic package import for thread-safe operations.Import of the atomic package enables thread-safe operations on the newly introduced
latestGasPricefield, which is a necessary addition for concurrent access to this value.
28-28: Added thread-safe gas price tracking.The introduction of
latestGasPriceas an atomic value ensures thread-safe storage and retrieval of the latest gas price, which is essential in a concurrent environment where multiple goroutines might access this value.
89-90: Improved variable naming for block number.Using a dedicated
blockNumvariable derived fromblockID.Seqnoenhances code readability by clearly indicating the purpose of this value when used in the subsequent API call.
94-98: Enhanced error handling in PostGasPrice.The error handling has been improved by directly checking the error returned from
PostVoteGasPriceand providing a more descriptive error message when wrapping it, which facilitates better debugging and error reporting.
99-99: Added gas price caching after successful posting.The method now calls
setLatestGasPriceto store the successful gas price for future reference, which is an important addition to maintain state consistency and potentially optimize future operations.
135-143: Implemented thread-safe gas price access methods.The new
getLatestGasPriceandsetLatestGasPricemethods provide a clean interface for accessing and updating the gas price in a thread-safe manner using atomic operations. The return value fromgetLatestGasPriceincludes a boolean indicating if a valid price is available, which is a robust design pattern.zetaclient/chains/base/observer_test.go (3)
549-573: Good test case for ballot not found scenario.The test effectively covers the case where a cross-chain transaction exists but the ballot is not found. It properly mocks the required dependencies and verifies both the return value and logging behavior.
575-598: Well-structured test for existing ballot scenario.This test complements the previous one by testing the case where both the context and ballot are found. The structured approach with clear arrange-act-assert pattern and verification of both return values and log messages is commendable.
527-528: Good enhancement to existing tests.Updating the existing test cases to include MockGetCctxByHash with appropriate error responses ensures that the tests properly simulate the "context not found" condition, maintaining consistency with the new test cases.
Also applies to: 541-542
zetaclient/testutils/mocks/zetacore_client_opts.go (3)
55-58: Well-implemented mock method for GetCctxByHash.The mock implementation follows the established pattern in the file and provides the necessary functionality for testing error handling scenarios.
60-63: Clean implementation of MockGetBallotByID.The method properly sets up the mock expectation and maintains the fluent interface pattern by returning the receiver.
48-53: Added explicit return statement for consistency.Adding the explicit return statement in WithPostVoteInbound method enhances code clarity and maintains consistency with other methods in the file.
zetaclient/testutils/mocks/zetacore_client.go (2)
143-171: Well-structured mock implementation for GetBallotByID.The implementation follows the standard mockery pattern with proper parameter validation, return value handling, and error management. This facilitates testing ballot-related scenarios.
201-229: Properly implemented GetCctxByHash mock.The mock method is consistent with other methods in the file and correctly implements error handling and return value processing, enabling effective testing of cross-chain transaction retrieval scenarios.
zetaclient/chains/ton/observer/outbound.go (7)
20-23: Good documentation for TON computation costs.The comment provides valuable context about the gas limit constant and its potential future changes, enhancing code maintainability.
215-218: Clear explanation for using zero block height.The comment explains why block height is set to zero for TON, providing essential context about TON's block structure and why conventional block height doesn't apply.
227-236: Important gas price validation added.Adding explicit gas price retrieval and validation prevents potential issues if the gas price hasn't been set, making the code more robust.
237-251: Improved message creation with appropriate parameters.The updated NewMsgVoteOutbound call uses the correct parameters and constants, enhancing code clarity and maintainability.
267-271: Enhanced error handling with cleaner control flow.Replacing the previous error handling with a more structured switch statement improves readability and maintainability.
31-31: Updated type references for consistency.Changing from cc.CrossChainTx to cctypes.CrossChainTx ensures consistency with other parts of the codebase and reflects the correct import naming.
Also applies to: 102-102
209-214: Simplified function signature.The updated postVoteOutbound signature now directly accepts the outbound object rather than individual parameters, reducing parameter count and improving code clarity.
|
pushed an empty commit because CI was not running for some reason |
4 participants
Backport #3711
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests