feat: enable multiple confirmation count values in chain params #3461
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 introduces significant changes across the codebase in version 27.0.0. The modifications include an updated changelog detailing breaking changes and new features, enhancements to API documentation with additional confirmation structures, and expanded Protocol Buffers definitions. The observer module now incorporates a new Confirmation field into the ChainParams structure along with additional validation checks, while migration functions and tests have been added to transition the state from consensus version 9 to 10. Test utilities and mocks have also been updated to include the new confirmation data. Changes
Sequence Diagram(s)sequenceDiagram
participant M as Migrator
participant V10 as v10.MigrateStore
participant OK as observerKeeper
participant DB as ChainParams Store
M->>V10: Call Migrate9to10(ctx)
V10->>OK: GetChainParamsList(ctx)
OK-->>V10: Return ChainParams list
V10->>V10: For each ChainParams update Confirmation fields<br/>(safe_inbound, fast_inbound, safe_outbound, fast_outbound)
V10->>OK: SetChainParamsList(updated list)
OK-->>V10: Acknowledge update
V10-->>M: Return success (nil)
Assessment against linked issues
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
|
ws4charlie
added
zetacore
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3461 +/- ##
===========================================
+ Coverage 65.29% 65.41% +0.11%
===========================================
Files 441 442 +1
Lines 30425 30521 +96
===========================================
+ Hits 19865 19964 +99
+ Misses 9704 9702 -2
+ Partials 856 855 -1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
x/observer/migrations/v10/migrate_test.go (3)
15-25: Consider adding more diverse test cases.The test data could be more comprehensive. Consider adding edge cases such as:
- Maximum allowed values for confirmation counts
- Different combinations of fast/safe counts
- Chain params with existing confirmation values
27-65: Add assertions for individual field migrations.While the test verifies overall equality, it would be beneficial to add specific assertions for each migrated field before the final comparison.
// compare the old and new chain params for i, newParam := range newChainParams.ChainParams { oldParam := oldChainParams.ChainParams[i] + + // Assert chain ID remains unchanged + require.Equal(t, oldParam.ChainId, newParam.ChainId) + + // Assert old confirmation count is correctly migrated + require.Equal(t, oldParam.ConfirmationCount, newParam.ConfirmationCount) // ensure the confirmation fields are set correctly require.Equal(t, newParam.Confirmation.SafeInboundCount, oldParam.ConfirmationCount)
86-92: Consider adding validation for confirmation values.The helper function could validate that confirmation counts are non-negative.
func makeChainParamsEmptyConfirmation(chainID int64, confirmationCount uint64) *types.ChainParams { + if confirmationCount < 0 { + panic("confirmation count must be non-negative") + } chainParams := sample.ChainParams(chainID) chainParams.ConfirmationCount = confirmationCount chainParams.Confirmation = types.Confirmation{} return chainParams }x/observer/types/chain_params_test.go (1)
160-177: Consider adding boundary test cases.While the validation tests are good, consider adding tests for:
- Maximum allowed values
- Equal safe/fast counts
- Zero fast count (disabled state)
x/observer/types/chain_params.go (2)
192-197: Extract common confirmation count values into constants.The confirmation count values are repeated across multiple chain configurations. Consider extracting these into named constants for better maintainability.
+const ( + DefaultEthereumConfirmationCount = 14 + DefaultBitcoinConfirmationCount = 2 + DefaultTestnetConfirmationCount = 6 + DefaultLocalnetConfirmationCount = 1 +) func GetDefaultEthMainnetChainParams() *ChainParams { return &ChainParams{ // ... Confirmation: Confirmation{ - SafeInboundCount: 14, - FastInboundCount: 14, - SafeOutboundCount: 14, - FastOutboundCount: 14, + SafeInboundCount: DefaultEthereumConfirmationCount, + FastInboundCount: DefaultEthereumConfirmationCount, + SafeOutboundCount: DefaultEthereumConfirmationCount, + FastOutboundCount: DefaultEthereumConfirmationCount, }, } }Also applies to: 216-221, 241-245, 266-270, 290-294, 314-318, 338-342, 362-366, 387-391
429-430: Implement explicit field comparison for Confirmation struct.Direct struct comparison might not be safe if the Confirmation struct is modified in the future. Consider comparing individual fields explicitly.
- params1.GatewayAddress == params2.GatewayAddress && - params1.Confirmation == params2.Confirmation + params1.GatewayAddress == params2.GatewayAddress && + params1.Confirmation.SafeInboundCount == params2.Confirmation.SafeInboundCount && + params1.Confirmation.FastInboundCount == params2.Confirmation.FastInboundCount && + params1.Confirmation.SafeOutboundCount == params2.Confirmation.SafeOutboundCount && + params1.Confirmation.FastOutboundCount == params2.Confirmation.FastOutboundCountdocs/openapi/openapi.swagger.yaml (1)
59812-59826: ObserverConfirmation Object Definition
The newly definedobserverConfirmationobject, with its four distinct properties (safe_inbound_count,fast_inbound_count,safe_outbound_count, andfast_outbound_count), is well structured. The use oftype: stringwithformat: uint64is a common pattern in APIs to safely represent large integers. However, please verify consistency with other similar definitions in your codebase, and ensure that clients correctly parse these values as unsigned 64-bit integers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
typescript/zetachain/zetacore/observer/confirmation_pb.d.tsis excluded by!**/*_pb.d.tstypescript/zetachain/zetacore/observer/params_pb.d.tsis excluded by!**/*_pb.d.tsx/observer/types/confirmation.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/observer/types/params.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (13)
changelog.md(1 hunks)docs/openapi/openapi.swagger.yaml(2 hunks)proto/zetachain/zetacore/observer/confirmation.proto(1 hunks)proto/zetachain/zetacore/observer/params.proto(2 hunks)testutil/sample/observer.go(2 hunks)typescript/zetachain/zetacore/observer/index.d.ts(1 hunks)x/observer/keeper/migrator.go(2 hunks)x/observer/migrations/v10/migrate.go(1 hunks)x/observer/migrations/v10/migrate_test.go(1 hunks)x/observer/module.go(2 hunks)x/observer/types/chain_params.go(11 hunks)x/observer/types/chain_params_test.go(3 hunks)zetaclient/testutils/mocks/chain_params.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
x/observer/keeper/migrator.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/confirmation.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
zetaclient/testutils/mocks/chain_params.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/module.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/chain_params.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/sample/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
proto/zetachain/zetacore/observer/params.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/observer/migrations/v10/migrate_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/chain_params_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/migrations/v10/migrate.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 Buf (1.47.2)
proto/zetachain/zetacore/observer/confirmation.proto
2-2: Files with package "zetachain.zetacore.observer" must be within a directory "zetachain/zetacore/observer" relative to root but were in directory "proto/zetachain/zetacore/observer".
(PACKAGE_DIRECTORY_MATCH)
6-11: Message "Confirmation" should have a non-empty comment for documentation.
(COMMENT_MESSAGE)
🔇 Additional comments (11)
typescript/zetachain/zetacore/observer/index.d.ts (1)
4-4: LGTM! Export follows established pattern.The new export for confirmation_pb aligns with the existing export structure and supports the new confirmation types.
x/observer/keeper/migrator.go (1)
60-63: LGTM! Migration function follows established pattern.The implementation correctly delegates to the v10 migration logic and maintains consistency with existing migration functions.
x/observer/migrations/v10/migrate_test.go (1)
67-83: LGTM! Good error case coverage.The test properly verifies the error case when chain params are not found.
x/observer/module.go (2)
155-157: LGTM! Migration registration follows established pattern.The migration registration follows the same pattern as previous migrations.
182-182: LGTM! Consensus version updated correctly.The consensus version is correctly incremented to 10.
x/observer/types/chain_params_test.go (1)
72-77: LGTM! Good test coverage for confirmation fields.The test setup properly initializes both EVM and BTC chain params with appropriate confirmation values.
Also applies to: 94-99
docs/openapi/openapi.swagger.yaml (1)
59802-59803: New Confirmation Field Addition
The newconfirmationfield is correctly added and references theobserverConfirmationdefinition. Ensure that downstream consumers are aware of this new field change for proper integration.proto/zetachain/zetacore/observer/confirmation.proto (1)
1-2: Directory Structure and Package Alignment.
Static analysis indicates that files declared with the packagezetachain.zetacore.observershould reside in a directory structured aszetachain/zetacore/observerrelative to the repository root. Since this file is located underproto/zetachain/zetacore/observer, please verify that this arrangement is intentional and consistent with your project’s organizational conventions.🧰 Tools
🪛 Buf (1.47.2)
2-2: Files with package "zetachain.zetacore.observer" must be within a directory "zetachain/zetacore/observer" relative to root but were in directory "proto/zetachain/zetacore/observer".
(PACKAGE_DIRECTORY_MATCH)
proto/zetachain/zetacore/observer/params.proto (2)
5-5: Updated Import of Confirmation Definition.
The import statement for"zetachain/zetacore/observer/confirmation.proto"has been added to incorporate the newConfirmationmessage. Please verify that the import path is correct relative to your project's proto search paths and aligns with your overall directory structure.
33-33: 🛠️ Refactor suggestionIntegrating the New Confirmation Field.
A new field of typeConfirmationhas been appended to theChainParamsmessage at line 33. While this addition provides enhanced granularity through multiple confirmation counts, note that an existing field (confirmation_countat line 13) is still present. If the intention is to replace the old aggregated count with the new structure, consider deprecating or removing theconfirmation_countfield to avoid ambiguity in future iterations. For example, you might add a deprecation option or comment to signal its planned removal.changelog.md (1)
18-18: Clear Changelog Entry for Multiple Confirmation Count Values.
The changelog entry at line 18 succinctly documents the feature update introduced by [PR #3461]. This new entry reflects the implementation of multiple confirmation count values in the chain parameters. It may be beneficial to ensure that any necessary migration details or usage notes are also referenced in your documentation elsewhere.
…ound, outbound count
…k 0 count any more
…t-add-multiple-confirmation-counts
…rate script and unit test
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
ws4charlie
added
the
CONSENSUS_BREAKING_ACK
Description
Added an extra
Confirmationfield to chain params struct so we could have fine-grained, separate control overinbound/outboundconfirmation counts andsafe/fastconfirmation counts.Closes: #3423
How Has This Been Tested?
Summary by CodeRabbit