feat: fast inbound confirmation for EVM and Bitcoin chain #3551
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 update implements inbound fast confirmation support for both EVM and Bitcoin chains. It adds new changelog entries, test cases, and methods to dynamically determine confirmation modes based on block numbers. Several tests have been revised to remove local block mining steps, and new functionalities were introduced in modules handling coins, liquidity cap calculations, and foreign coin queries. Additionally, the client configuration has been restructured to use CometBFT terminology instead of Tendermint, and logging is enhanced with additional context. Changes
Sequence Diagram(s)sequenceDiagram
participant Observer
participant BlockScanner
participant TxServer
participant Ledger
Observer->>BlockScanner: Request SAFE block range
BlockScanner-->>Observer: Return SAFE range (startSafe, endSafe)
Observer->>BlockScanner: Request FAST block range
BlockScanner-->>Observer: Return FAST range (startFast, endFast)
Observer->>TxServer: Determine confirmation mode using block number
TxServer-->>Observer: Return confirmation mode (FAST or SAFE)
Observer->>Ledger: Post inbound vote with determined mode
Ledger-->>Observer: Acknowledge vote
sequenceDiagram
participant TestRunner
participant ChainParams
participant DepositProcessor
participant TxServer
TestRunner->>ChainParams: Update chain parameters for fast confirmation
ChainParams-->>TestRunner: Confirm parameters updated
TestRunner->>DepositProcessor: Trigger fast confirmation deposit test
DepositProcessor->>TxServer: Process deposit within fast cap
TxServer-->>DepositProcessor: Return fast confirmation result
DepositProcessor->>TestRunner: Assert deposit confirmed (FAST or SAFE)
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 #3551 +/- ##
===========================================
- Coverage 64.69% 64.67% -0.02%
===========================================
Files 466 468 +2
Lines 32477 32608 +131
===========================================
+ Hits 21010 21090 +80
- Misses 10512 10561 +49
- Partials 955 957 +2
|
ws4charlie
added
zetaclient
ws4charlie
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (22)
x/crosschain/types/message_vote_inbound.go (1)
159-180: LGTM! Well-structured implementation with clear logic.The function effectively determines fast confirmation eligibility with proper handling of different protocol versions and clear documentation of the reasoning.
Consider defining protocol version constants and standardizing comment format:
+// Protocol version constants +const ( + ProtocolV1 = ProtocolContractVersion_V1 + ProtocolV2 = ProtocolContractVersion_V2 +) func (msg *MsgVoteInbound) EligibleForFastConfirmation() bool { - // only fungible coins are eligible for fast confirmation + // Only fungible coins are eligible for fast confirmation if !msg.CoinType.IsFungible() { return false } switch msg.ProtocolContractVersion { - case ProtocolContractVersion_V1: + case ProtocolV1: // msg using protocol contract version V1 is not eligible for fast confirmation because: // 1. whether the receiver address is a contract or not is unknown // 2. it can be a depositAndCall (Gas or ZRC20) with empty payload // 3. it can be a message passing (CoinType_Zeta) calls 'onReceive' return false - case ProtocolContractVersion_V2: + case ProtocolV2: // in protocol contract version V2, simple deposit is distinguished from depositAndCall/NoAssetCall return !msg.IsCrossChainCall default: return false } }x/crosschain/types/message_vote_inbound_test.go (1)
625-671: LGTM! Comprehensive test coverage with clear test cases.The test suite effectively covers the main scenarios for fast confirmation eligibility.
Consider adding these additional test cases for better coverage:
func TestMsgVoteInbound_EligibleForFastConfirmation(t *testing.T) { tests := []struct { name string msg types.MsgVoteInbound eligible bool }{ + { + name: "not eligible when cross chain call is true", + msg: func() types.MsgVoteInbound { + msg := sample.InboundVote(coin.CoinType_Gas, 1, 7000) + msg.ProtocolContractVersion = types.ProtocolContractVersion_V2 + msg.IsCrossChainCall = true + return msg + }(), + eligible: false, + }, + { + name: "eligible for different fungible coin types", + msg: func() types.MsgVoteInbound { + msg := sample.InboundVote(coin.CoinType_ERC20, 1, 7000) + msg.ProtocolContractVersion = types.ProtocolContractVersion_V2 + return msg + }(), + eligible: true, + }, // existing test cases... }zetaclient/chains/base/observer.go (1)
383-426: Consider enhancing error handling and metrics.While the implementation is solid, consider these improvements:
- Add specific error types for different failure scenarios to aid in error tracking and resolution.
- Include metrics to track the distribution of FAST vs SAFE confirmation modes.
Example implementation:
// PostVoteInbound posts a vote for the given vote message and returns the ballot. func (ob *Observer) PostVoteInbound( ctx context.Context, msg *crosschaintypes.MsgVoteInbound, retryGasLimit uint64, ) (string, error) { const gasLimit = zetacore.PostVoteInboundGasLimit + + // Track confirmation mode distribution + metrics.InboundConfirmationModes.WithLabelValues( + ob.chain.Name, + msg.ConfirmationMode.String(), + ).Inc() + var ( txHash = msg.InboundHash coinType = msg.CoinType )x/fungible/keeper/grpc_query_foreign_coins.go (1)
67-82: Enhance error handling and input validation.The error messages could be more descriptive, and the method would benefit from input validation for the chain ID and asset format.
func (k Keeper) ForeignCoinsFromAsset( c context.Context, req *types.QueryGetForeignCoinsFromAssetRequest, ) (*types.QueryGetForeignCoinsFromAssetResponse, error) { if req == nil { return nil, status.Error(codes.InvalidArgument, "invalid request") } + if req.ChainId <= 0 { + return nil, status.Error(codes.InvalidArgument, "chain ID must be positive") + } + if req.Asset == "" { + return nil, status.Error(codes.InvalidArgument, "asset address cannot be empty") + } ctx := sdk.UnwrapSDKContext(c) fCoin, found := k.GetForeignCoinFromAsset(ctx, req.Asset, req.ChainId) if !found { - return nil, status.Error(codes.NotFound, "not found") + return nil, status.Errorf(codes.NotFound, "foreign coin not found for asset %s on chain %d", req.Asset, req.ChainId) } return &types.QueryGetForeignCoinsFromAssetResponse{ForeignCoins: fCoin}, nil }zetaclient/zetacore/client_fungible_test.go (2)
20-41: Add test cases for invalid chain IDs and malformed addresses.The test suite would benefit from additional test cases to cover invalid inputs and error scenarios.
tests := []struct { name string chainID int64 asset string errMsg string }{ // existing test cases... + { + name: "invalid chain ID", + chainID: -1, + asset: erc20Asset, + errMsg: "chain ID must be positive", + }, + { + name: "malformed asset address", + chainID: 1, + asset: "0xinvalid", + errMsg: "invalid asset address format", + }, }
88-115: Improve test organization and naming.This test case could be integrated into the table-driven test suite above with a more descriptive name.
Consider merging this test into the table-driven test suite in
Test_GetForeignCoinsFromAssetwith a descriptive name like "successful retrieval of ERC20 foreign coins".pkg/constant/fast_confirmation_multiplier.go (2)
16-57: Consider a more maintainable approach for chain management.The current implementation requires explicit addition of new chains to the map. This could lead to maintenance overhead and potential issues if a new chain is added without updating this map.
Consider implementing a more dynamic approach, such as:
- Using a configuration file that can be updated without code changes
- Implementing a default behavior for new chains
- Adding runtime configuration capabilities through admin APIs
68-72: Document truncation behavior in amount cap calculation.The function truncates decimal places without rounding, which might lead to unexpected behavior for values close to the next integer.
Add a comment explaining the truncation behavior:
// CalcInboundFastAmountCap calculates the amount cap for inbound fast confirmation. +// Note: The result is truncated (not rounded) to the nearest integer towards zero. func CalcInboundFastAmountCap(liquidityCap sdkmath.Uint, multiplier sdkmath.LegacyDec) *big.Int {pkg/constant/fast_confirmation_multiplier_test.go (2)
13-72: Enhance test coverage for edge cases.Consider adding test cases for:
- Non-existent chain IDs to verify the behavior
- Chain IDs at the boundaries of int64
Add the following test cases:
tests := []struct { name string chainID int64 expected sdkmath.LegacyDec }{ + // Edge cases + {name: "Non-existent chain", chainID: 999999, expected: sdkmath.LegacyDec{}}, + {name: "Maximum chain ID", chainID: 9223372036854775807, expected: sdkmath.LegacyDec{}}, + {name: "Minimum chain ID", chainID: -9223372036854775808, expected: sdkmath.LegacyDec{}},
74-113: Add descriptive comments for test cases.The test cases would benefit from comments explaining the test scenarios and expected behaviors.
Add descriptive comments:
tests := []struct { name string liquidityCap sdkmath.Uint multiplier sdkmath.LegacyDec expected *big.Int }{ + // Test standard percentage calculation { name: "1% of 10000", liquidityCap: sdkmath.NewUintFromString("10000"), multiplier: sdkmath.LegacyMustNewDecFromStr("0.01"), expected: big.NewInt(100), }, + // Test handling of small percentages { name: "0.15% of 10000",e2e/e2etests/test_eth_deposit_fast_confirmation.go (1)
54-56: Consider making the liquidity cap percentage configurable.The hardcoded 50% increase for liquidity cap could be made configurable to allow for different test scenarios without code changes.
+const defaultLiquidityCapIncrease = 50 // set ZRC20 liquidity cap to 150% of the current supply // note: the percentage should not be too small as it may block other tests -liquidityCap, _ := mathpkg.IncreaseUintByPercent(supplyUint, 50) +liquidityCap, _ := mathpkg.IncreaseUintByPercent(supplyUint, defaultLiquidityCapIncrease)zetaclient/chains/base/confirmation.go (1)
72-103: Enhance error handling for better observability.While the function logic is sound, consider improving error handling:
func (ob *Observer) IsInboundEligibleForFastConfirmation( ctx context.Context, msg *crosschaintypes.MsgVoteInbound, ) (bool, error) { chainID := msg.SenderChainId multiplier, enabled := constant.GetInboundFastConfirmationLiquidityMultiplier(chainID) if !enabled { - return false, nil + return false, errors.Errorf("fast confirmation not enabled for chain %d", chainID) } if !msg.EligibleForFastConfirmation() { - return false, nil + return false, errors.New("message not eligible for fast confirmation") } fCoins, err := ob.zetacoreClient.GetForeignCoinsFromAsset(ctx, chainID, msg.Asset) if err != nil { return false, errors.Wrapf(err, "unable to get foreign coins for asset %s on chain %d", msg.Asset, chainID) } fastAmountCap := constant.CalcInboundFastAmountCap(fCoins.LiquidityCap, multiplier) if msg.Amount.BigInt().Cmp(fastAmountCap) > 0 { - return false, nil + return false, errors.Errorf("amount %s exceeds fast confirmation cap %s", msg.Amount, fastAmountCap) } return true, nil }zetaclient/chains/bitcoin/observer/event.go (1)
186-188: Consider extracting confirmation mode determination to reduce duplication.The confirmation mode determination logic is duplicated in both
NewInboundVoteFromLegacyMemoandNewInboundVoteFromStdMemo. Consider extracting this into a helper method to improve maintainability.+func (ob *Observer) getConfirmationModeForEvent(event *BTCInboundEvent) crosschaintypes.ConfirmationMode { + return ob.GetInboundConfirmationMode(event.BlockNumber) +}Then use it in both functions:
-confirmationMode := ob.GetInboundConfirmationMode(event.BlockNumber) +confirmationMode := ob.getConfirmationModeForEvent(event)Also applies to: 227-229
zetaclient/chains/base/confirmation_test.go (2)
226-265: Add test cases for edge cases inTest_GetInboundConfirmationMode.The test cases only cover basic scenarios. Consider adding test cases for:
- Block number at the boundary of SAFE/FAST confirmation
- Zero block number
- Maximum block number
351-442: Enhance test coverage forTest_IsInboundEligibleForFastConfirmation.The test cases are well-structured but could be more comprehensive. Consider adding test cases for:
- Different coin types
- Edge cases for amount calculations
- Different protocol contract versions
Additionally, consider extracting the test setup into a helper function to improve readability:
func setupTestInboundEligibility(t *testing.T, chain chains.Chain, liquidityCap sdkmath.Uint) *testSuite { ob := newTestSuite(t, chain) ob.zetacore.On("GetForeignCoinsFromAsset", mock.Anything, chain.ChainId, mock.Anything). Maybe(). Return(fungibletypes.ForeignCoins{LiquidityCap: liquidityCap}, nil) return ob }e2e/runner/bitcoin.go (1)
284-285: Document the change in confirmation block count.The reduction in confirmation blocks from 6 to 1 is a significant change that should be documented. Consider adding a comment explaining why this change was made and its implications for fast confirmation.
-// mine 1 block to confirm the transaction +// mine 1 block to confirm the transaction +// Note: Reduced from 6 blocks to 1 block to support fast confirmation featurezetaclient/chains/bitcoin/observer/event_test.go (2)
370-372: Add test cases for different confirmation modes.The test only mocks SAFE confirmation mode. Consider adding test cases for:
- FAST confirmation mode
- Edge cases between SAFE and FAST modes
428-430: Add test cases for different confirmation modes.Similar to the previous test, this test only mocks SAFE confirmation mode. Consider adding test cases for:
- FAST confirmation mode
- Edge cases between SAFE and FAST modes
zetaclient/chains/evm/observer/inbound.go (1)
133-137: Consider adding error handling and logging for FAST block scanning.The FAST block scanning implementation could benefit from similar error handling and logging as the SAFE block scanning section.
// scan FAST confirmed blocks if available _, endBlockFast := ob.GetScanRangeInboundFast(config.MaxBlocksPerScan) if endBlockSafe < endBlockFast { - ob.observeInboundInBlockRange(ctx, endBlockSafe, endBlockFast-1) + lastScannedNew := ob.observeInboundInBlockRange(ctx, endBlockSafe, endBlockFast-1) + logger.Debug(). + Uint64("from", endBlockSafe). + Uint64("to", lastScannedNew). + Msg("observed blocks for fast inbounds") }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 134-136: zetaclient/chains/evm/observer/inbound.go#L134-L136
Added lines #L134 - L136 were not covered by testsdocs/openapi/openapi.swagger.yaml (2)
29453-29478: Endpoint Definition and Parameter ValidationThe new endpoint
/zeta-chain/fungible/foreign_coins/{chain_id}/{asset}is defined clearly with an appropriate GET method, summary, operationId, responses, and path parameters. The inclusion of both required parameters (chain_idandasset) is correctly handled.One optional suggestion: Consider reviewing the data type for
chain_id. Although it is currently defined as a string withformat: int64, many OpenAPI definitions prefer using an integer type for numeric IDs. If your backend expects a numeric value, you might update it as follows:- type: string - format: int64 + type: integer + format: int64If the chain identifier is indeed a string in your context, then the current configuration is acceptable.
59614-59624: Response Schema ImplementationThe new schema
fungibleQueryGetForeignCoinsFromAssetResponsehas been added properly. Defining the response as an object with a propertyforeignCoinsthat references thefungibleForeignCoinsdefinition is clear and maintains consistency with existing patterns.For enhanced clarity to API consumers, consider adding a brief
descriptionfor theforeignCoinsproperty to summarize its contents. This additional documentation could improve usability of the API.go.mod (1)
313-314: Confirm Dependency Duplication Intent
The new dependency"github.com/test-go/testify v1.1.4"appears to be added alongside an existing dependency"github.com/stretchr/testify v1.10.0". This could lead to potential conflicts or redundancy unless the new import is intentionally meant to serve a different purpose. Please verify if both dependencies are required or if this is an oversight. Consider aligning to a single version if possible.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
typescript/zetachain/zetacore/fungible/query_pb.d.tsis excluded by!**/*_pb.d.tsx/fungible/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/fungible/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.go
📒 Files selected for processing (46)
changelog.md(1 hunks)cmd/zetae2e/local/bitcoin.go(2 hunks)cmd/zetae2e/local/evm.go(1 hunks)docs/openapi/openapi.swagger.yaml(2 hunks)e2e/e2etests/e2etests.go(4 hunks)e2e/e2etests/helpers.go(0 hunks)e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go(0 hunks)e2e/e2etests/test_bitcoin_deposit_call.go(0 hunks)e2e/e2etests/test_bitcoin_deposit_fast_confirmation.go(1 hunks)e2e/e2etests/test_bitcoin_donation.go(0 hunks)e2e/e2etests/test_bitcoin_std_deposit.go(0 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call.go(0 hunks)e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go(0 hunks)e2e/e2etests/test_crosschain_swap.go(0 hunks)e2e/e2etests/test_eth_deposit_fast_confirmation.go(1 hunks)e2e/e2etests/test_eth_deposit_liquidity_cap.go(2 hunks)e2e/runner/bitcoin.go(1 hunks)e2e/txserver/liquidity_cap.go(1 hunks)go.mod(1 hunks)pkg/coin/coin.go(1 hunks)pkg/coin/coin_test.go(1 hunks)pkg/constant/fast_confirmation_multiplier.go(1 hunks)pkg/constant/fast_confirmation_multiplier_test.go(1 hunks)proto/zetachain/zetacore/fungible/query.proto(2 hunks)x/crosschain/types/message_vote_inbound.go(1 hunks)x/crosschain/types/message_vote_inbound_test.go(1 hunks)x/fungible/keeper/foreign_coins_test.go(1 hunks)x/fungible/keeper/grpc_query_foreign_coins.go(1 hunks)x/fungible/keeper/grpc_query_foreign_coins_test.go(7 hunks)zetaclient/chains/base/confirmation.go(3 hunks)zetaclient/chains/base/confirmation_test.go(5 hunks)zetaclient/chains/base/observer.go(1 hunks)zetaclient/chains/bitcoin/observer/event.go(4 hunks)zetaclient/chains/bitcoin/observer/event_test.go(2 hunks)zetaclient/chains/bitcoin/observer/inbound.go(2 hunks)zetaclient/chains/evm/observer/inbound.go(1 hunks)zetaclient/chains/evm/observer/v2_inbound.go(3 hunks)zetaclient/chains/interfaces/interfaces.go(2 hunks)zetaclient/logs/fields.go(1 hunks)zetaclient/testutils/mocks/zetacore_client.go(2 hunks)zetaclient/zetacore/broadcast_test.go(2 hunks)zetaclient/zetacore/client.go(5 hunks)zetaclient/zetacore/client_fungible.go(1 hunks)zetaclient/zetacore/client_fungible_test.go(1 hunks)zetaclient/zetacore/client_test.go(4 hunks)zetaclient/zetacore/tx_test.go(8 hunks)
💤 Files with no reviewable changes (8)
- e2e/e2etests/test_bitcoin_donation.go
- e2e/e2etests/test_bitcoin_std_deposit.go
- e2e/e2etests/helpers.go
- e2e/e2etests/test_bitcoin_deposit_and_call_revert_with_dust.go
- e2e/e2etests/test_crosschain_swap.go
- e2e/e2etests/test_bitcoin_deposit_call.go
- e2e/e2etests/test_bitcoin_std_memo_inscribed_deposit_and_call.go
- e2e/e2etests/test_bitcoin_std_deposit_and_call.go
✅ Files skipped from review due to trivial changes (2)
- zetaclient/zetacore/client_test.go
- changelog.md
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.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.
e2e/runner/bitcoin.gocmd/zetae2e/local/evm.gozetaclient/zetacore/tx_test.gox/crosschain/types/message_vote_inbound.gopkg/coin/coin.gozetaclient/chains/bitcoin/observer/event_test.gozetaclient/chains/base/observer.goe2e/e2etests/test_eth_deposit_liquidity_cap.gozetaclient/zetacore/client_fungible.gozetaclient/chains/bitcoin/observer/event.gocmd/zetae2e/local/bitcoin.goe2e/e2etests/test_eth_deposit_fast_confirmation.gozetaclient/chains/base/confirmation.gozetaclient/chains/evm/observer/inbound.gopkg/coin/coin_test.goe2e/txserver/liquidity_cap.gox/crosschain/types/message_vote_inbound_test.goe2e/e2etests/test_bitcoin_deposit_fast_confirmation.gox/fungible/keeper/grpc_query_foreign_coins.gopkg/constant/fast_confirmation_multiplier_test.gozetaclient/zetacore/broadcast_test.gozetaclient/logs/fields.gox/fungible/keeper/grpc_query_foreign_coins_test.gozetaclient/chains/base/confirmation_test.gozetaclient/chains/bitcoin/observer/inbound.gozetaclient/chains/interfaces/interfaces.gozetaclient/zetacore/client_fungible_test.gozetaclient/zetacore/client.gozetaclient/chains/evm/observer/v2_inbound.gopkg/constant/fast_confirmation_multiplier.gox/fungible/keeper/foreign_coins_test.gozetaclient/testutils/mocks/zetacore_client.goe2e/e2etests/e2etests.go
`**/*.proto`: Review the Protobuf definitions, point out iss...
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/fungible/query.proto
🧠 Learnings (1)
zetaclient/chains/bitcoin/observer/inbound.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:131-132
Timestamp: 2024-11-12T13:20:12.658Z
Learning: ObserveInbound coverage will be improved in future refactor.
🪛 GitHub Check: codecov/patch
zetaclient/zetacore/client_fungible.go
[warning] 33-39: zetaclient/zetacore/client_fungible.go#L33-L39
Added lines #L33 - L39 were not covered by tests
zetaclient/chains/evm/observer/inbound.go
[warning] 116-129: zetaclient/chains/evm/observer/inbound.go#L116-L129
Added lines #L116 - L129 were not covered by tests
[warning] 134-136: zetaclient/chains/evm/observer/inbound.go#L134-L136
Added lines #L134 - L136 were not covered by tests
zetaclient/chains/bitcoin/observer/inbound.go
[warning] 32-42: zetaclient/chains/bitcoin/observer/inbound.go#L32-L42
Added lines #L32 - L42 were not covered by tests
[warning] 45-52: zetaclient/chains/bitcoin/observer/inbound.go#L45-L52
Added lines #L45 - L52 were not covered by tests
[warning] 57-65: zetaclient/chains/bitcoin/observer/inbound.go#L57-L65
Added lines #L57 - L65 were not covered by tests
[warning] 110-121: zetaclient/chains/bitcoin/observer/inbound.go#L110-L121
Added lines #L110 - L121 were not covered by tests
zetaclient/zetacore/client.go
[warning] 137-137: zetaclient/zetacore/client.go#L137
Added line #L137 was not covered by tests
zetaclient/chains/evm/observer/v2_inbound.go
[warning] 109-120: zetaclient/chains/evm/observer/v2_inbound.go#L109-L120
Added lines #L109 - L120 were not covered by tests
[warning] 201-202: zetaclient/chains/evm/observer/v2_inbound.go#L201-L202
Added lines #L201 - L202 were not covered by tests
[warning] 221-221: zetaclient/chains/evm/observer/v2_inbound.go#L221
Added line #L221 was not covered by tests
🔇 Additional comments (31)
zetaclient/chains/base/observer.go (2)
398-401: LGTM! Enhanced logging for confirmation modes.The addition of
logs.FieldConfirmationModeimproves observability by explicitly logging the confirmation mode (FAST or SAFE) for inbound votes, which aligns well with the PR's objective of implementing fast inbound confirmation support.
383-426:❓ Verification inconclusive
Verify confirmation mode propagation and logging consistency.
Please ensure:
- The confirmation mode is properly propagated through all relevant components.
- The logging fields for confirmation modes are consistently used across the codebase.
Run this script to check logging consistency:
🏁 Script executed:
#!/bin/bash # Check for consistent use of confirmation mode logging across the codebase rg -A 2 "FieldConfirmationMode"Length of output: 345
Verification of Confirmation Mode Propagation & Logging Consistency
The recent script execution confirms that the
FieldConfirmationModelogging field is used consistently in the codebase (e.g., inzetaclient/chains/base/observer.goandzetaclient/logs/fields.go). However, please ensure that the confirmation mode itself is appropriately propagated throughout all relevant system components—as the logging check does not fully verify the underlying propagation logic.
- Confirm that the value returned by
msg.ConfirmationMode.String()is correctly handled in all downstream components.- Double-check that any other modules or services relying on confirmation mode have been updated accordingly.
zetaclient/chains/bitcoin/observer/inbound.go (4)
31-42: Consider improving test coverage for safe inbound scanning.
The logic for scanning SAFE confirmed blocks and handling errors is mostly sound, yet lines 32-42 remain uncovered by tests. It would be beneficial to add integration tests that verify the behavior when:
startBlockSafeis >=endBlockSafe(no scanning).- An error occurs within
[startBlockSafe, endBlockSafe-1].🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-42: zetaclient/chains/bitcoin/observer/inbound.go#L32-L42
Added lines #L32 - L42 were not covered by tests
44-53: Persisting the last scanned block is sound, but ensure edge cases are tested.
Persisting the updated last scanned block only if it exceeds the current record is a good approach. However, these lines are also not covered by tests. Consider adding a test that verifies the block-saving logic when no inbound transactions are observed or when errors prevent scanning all blocks in the range.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-52: zetaclient/chains/bitcoin/observer/inbound.go#L45-L52
Added lines #L45 - L52 were not covered by tests
56-67: Add tests for FAST confirmation scanning.
The FAST confirmation logic at lines 56-67 is well-structured, but it appears untested. Consider adding unit or integration tests that simulate scenarios whereendBlockFastis both just below and aboveendBlockSafe, ensuring correct behavior and error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-65: zetaclient/chains/bitcoin/observer/inbound.go#L57-L65
Added lines #L57 - L65 were not covered by tests
110-123: Validate fast confirmation eligibility checks through dedicated test scenarios.
Skipping inbounds that are not eligible for fast confirmation is crucial logic. Currently, lines 110-121 lack coverage, making it difficult to ensure correctness in edge cases. Recommend adding tests that confirm inbound transactions are properly skipped or returned ifIsInboundEligibleForFastConfirmationreturns false or an error.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-121: zetaclient/chains/bitcoin/observer/inbound.go#L110-L121
Added lines #L110 - L121 were not covered by testszetaclient/logs/fields.go (1)
6-18: New field key aligns with enhanced confirmation logic.
The addition ofFieldConfirmationModeis consistent with the broader fast confirmation enhancement. No issues detected.zetaclient/zetacore/client_fungible.go (2)
19-24: Reasonable approach for handling gas assets.
Using an empty string when the asset address is a zero address effectively differentiates standard assets from gas assets. This design is clear and maintainable.
31-39: Test the error handling path to improve coverage.
IfFungible.ForeignCoinsFromAssetfails, lines 33-39 wrap and return the error. Consider adding a test case that triggers an error response fromFungible.ForeignCoinsFromAssetto ensure the wrapping and early return paths are exercised.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-39: zetaclient/zetacore/client_fungible.go#L33-L39
Added lines #L33 - L39 were not covered by testse2e/e2etests/test_eth_deposit_liquidity_cap.go (1)
26-26: LGTM! Improved code clarity and efficiency.The changes simplify the liquidity cap management by replacing message-based transactions with direct method calls, reducing complexity and improving maintainability.
Also applies to: 65-65
pkg/constant/fast_confirmation_multiplier.go (1)
11-14: Verify the default multiplier value.The default multiplier of 0.00025 (0.025%) seems quite small. This could significantly limit the fast confirmation capability for large transactions.
Please confirm if this restrictive multiplier aligns with the business requirements and performance expectations.
zetaclient/zetacore/broadcast_test.go (1)
83-83: LGTM! Consistent terminology update.The changes correctly update the test setup to use CometBFT terminology instead of Tendermint, maintaining consistency across the codebase.
Also applies to: 97-99
e2e/e2etests/test_eth_deposit_fast_confirmation.go (2)
67-68: LGTM: Clear test case for fast confirmation.The test case properly validates fast confirmation for deposits within the cap limit.
80-82: LGTM: Comprehensive test for safe confirmation.The test case effectively validates that deposits exceeding the cap trigger safe confirmation mode.
e2e/e2etests/test_bitcoin_deposit_fast_confirmation.go (1)
83-85: LGTM: Proper cleanup of mining routine.The test correctly uses defer to ensure the mining routine is stopped after the test.
pkg/coin/coin_test.go (1)
153-173: LGTM: Comprehensive test coverage for IsFungible method.The test cases cover all coin types and edge cases, following consistent test patterns.
cmd/zetae2e/local/evm.go (1)
21-21: LGTM: Proper integration of fast confirmation test.The test is correctly integrated into the ETH happy path test suite.
x/fungible/keeper/grpc_query_foreign_coins_test.go (3)
17-65: LGTM!The test function is well-structured with comprehensive test cases covering successful queries, not found scenarios, and invalid requests.
67-116: LGTM!The test function is well-implemented with comprehensive test cases that validate the
ForeignCoinsFromAssetquery functionality, including edge cases and error scenarios.
118-171: LGTM!The test function thoroughly validates pagination functionality with comprehensive test cases for offset-based, key-based, and total count queries.
zetaclient/chains/base/confirmation.go (1)
47-55: LGTM!The function provides a clean implementation for determining the confirmation mode based on block confirmation status.
cmd/zetae2e/local/bitcoin.go (1)
27-40: LGTM!The addition of the fast confirmation test case aligns with the PR objectives and maintains consistent test organization.
x/fungible/keeper/foreign_coins_test.go (1)
19-28: LGTM!The refactoring improves test maintainability by utilizing the sample package for consistent test data generation.
zetaclient/chains/interfaces/interfaces.go (1)
67-67: LGTM! Well-designed interface method.The new method
GetForeignCoinsFromAssetfollows Go interface design principles with clear parameter names and proper error handling.zetaclient/zetacore/client.go (1)
61-62: LGTM! Consistent renaming from Tendermint to CometBFT.The renaming from Tendermint to CometBFT is consistent throughout the file, improving code clarity and reflecting the current terminology.
Also applies to: 70-76, 302-304
zetaclient/zetacore/tx_test.go (1)
102-102: LGTM! Consistent test updates for CometBFT changes.The test cases have been properly updated to use
withCometBFTinstead ofwithTendermint, maintaining consistency with the main code changes. The test coverage remains comprehensive, including both success and error scenarios.Also applies to: 149-149, 176-176, 200-200, 237-237, 278-278, 315-315, 348-348
zetaclient/chains/evm/observer/inbound.go (1)
115-131:❓ Verification inconclusive
LGTM! Clean implementation of SAFE block scanning.
The code effectively handles SAFE confirmed blocks with proper error handling and logging.
Please ensure test coverage is added for this critical section:
🏁 Script executed:
#!/bin/bash # Check test coverage for the ObserveInbound function go test -coverprofile=coverage.out ./zetaclient/chains/evm/observer/ go tool cover -func=coverage.out | grep ObserveInboundLength of output: 17972
SAFE Block Scanning Implementation Verified – Further Test Coverage Verification Required
The implementation for scanning SAFE-confirmed blocks (lines 115–131) is clean, with proper error handling and logging in place. However, the automated test coverage command failed to produce any output due to build errors in other parts of the code (e.g., issues in go-ethereum and pkg/coin/pkg/chains), which prevents reliable verification of test coverage for the
ObserveInboundfunction.
- Action Required:
- Manually verify and resolve the build errors reported by the test suite.
- Ensure comprehensive test coverage is added for the
ObserveInboundfunction, either by fixing the dependencies causing the build failures or by isolating and testing this code separately.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 116-129: zetaclient/chains/evm/observer/inbound.go#L116-L129
Added lines #L116 - L129 were not covered by testszetaclient/testutils/mocks/zetacore_client.go (1)
13-14: LGTM! Well-structured mock implementation.The new mock method follows the established patterns and includes proper error handling.
Also applies to: 257-283
e2e/e2etests/e2etests.go (1)
16-16: LGTM! Well-organized test additions.The new test entries for fast confirmation are properly structured and placed in their respective sections (ETH and Bitcoin).
Also applies to: 92-92, 240-245, 689-694
proto/zetachain/zetacore/fungible/query.proto (2)
28-34: Enhance RPC Addition Verification
The addition of theForeignCoinsFromAssetRPC looks well-structured and adheres to RESTful design with a clear HTTP mapping (/zeta-chain/fungible/foreign_coins/{chain_id}/{asset}). Ensure that related server logic and client calls are updated accordingly to handle type validations and error conditions for the new parameters.
83-91: Validate New Message Definitions Consistency
The definitions forQueryGetForeignCoinsFromAssetRequestandQueryGetForeignCoinsFromAssetResponseare clear and consistent with existing patterns. Please double-check that downstream components (e.g., keeper implementations and test cases) correctly integrate these message types.
| } | ||
|
|
||
| // ensure the deposit amount does not exceed amount cap | ||
| fastAmountCap := constant.CalcInboundFastAmountCap(fCoins.LiquidityCap, multiplier) |
There was a problem hiding this comment.
Q: does EACH pending cctx for chain_id+asset decrease the liquidity cap?
There was a problem hiding this comment.
no, it is a static number which can only be manually updated with proposal.
There was a problem hiding this comment.
Let's consider a scenario where we have 100 ETH in a pool and allow fast cctx for deposits up to 0.05 ETH.
Now, imagine we have 1000 pending cctx, which would result in a deposit of 50 ETH (50% of the pool's value). Isn't it too risky to consider all of these cctx "fast"?
I believe each deposit should reduce the "fwindow" of pending cctx "fast" liquidity cap.
cc @lumtis
There was a problem hiding this comment.
I think you probably mean some rate limit logic. There was a discussion under 3458 to protect against chain-level attack.
…st confirmation E2E
If fast observation is disabled halfway, the cctx will be eventually be created using |
…lace argument type string with common.Address
…set string with common.Address
…ove divisor related file to chains pkg; renaming and comments
…t-fast-confirmation-inbound-evm-btc
There was a problem hiding this comment.
The flow is unclear to me and might be simplified imo
vote creation -> you check the inbound is elligible for fast observation with the amount, if so, set ConfirmationMode_FAST
When ConfirmationMode_FAST is set then when you get the number of block confirmed with the fast value, the vote get sent
| if lastBlock < lastScanned+confirmation { | ||
| return 0, 0 | ||
| return nextBlock, nextBlock |
There was a problem hiding this comment.
The pair of (0, 0) gives no info about where we are now? Returning (nextBlock, nextBlock) tells the caller the next block to scan. The caller uses it to calculate FAST block range.
| if msg.ConfirmationMode == types.ConfirmationMode_FAST { | ||
| eligible, err := ob.IsInboundEligibleForFastConfirmation(ctx, &msg) |
There was a problem hiding this comment.
Why do we check IsInboundEligibleForFastConfirmation when ConfirmationMode is true?
ConfirmationMode should be set at the inbound creation when IsInboundEligibleForFastConfirmation returns true
There was a problem hiding this comment.
The reason is that we need to skip posting vote if the msg carries FAST mode but not eligible for fast confirmation.
| confirmationMode := types.ConfirmationMode_FAST | ||
| if ob.IsBlockConfirmedForInboundSafe(event.Raw.BlockNumber) { | ||
| confirmationMode = types.ConfirmationMode_SAFE | ||
| } |
There was a problem hiding this comment.
This is when IsInboundEligibleForFastConfirmation should be used
It is unclear to me why we use IsBlockConfirmedForInboundSafe
There was a problem hiding this comment.
The same situation for caller. The caller (holding the vote message) needs to decide skip the vote or not. By default, the confirmation should be already FAST when logic arrives here. If the BlockNumber has safe confirmation count, then it's marked as SAFE.
Co-authored-by: Lucas Bertrand <[email protected]>
…t-fast-confirmation-inbound-evm-btc
Description
This PR is still in the progress.
GetScanRangeInboundSafeandGetScanRangeInboundFastsplit blocks intoSAFErange andFASTrange and scan them separately.ForeignCoinsFromAssetto fetch liquidity pool cap for given chain id and asset.IsInboundEligibleForFastConfirmationto determine if an inbound can be fast confirmed.FAST | SAFEto messageMsgVoteInbound.tendermintwithCometBFTin the zetaclient code.Closes: #3458
Closes: #3459
How Has This Been Tested?
Summary by CodeRabbit
New Features
Refactor