feat: whitelist connection gater #3028
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3028 +/- ##
===========================================
- Coverage 64.47% 64.46% -0.01%
===========================================
Files 412 412
Lines 28985 28986 +1
===========================================
Hits 18687 18687
- Misses 9515 9516 +1
Partials 783 783
|
|
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 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates across multiple files, primarily focusing on version 21.0.0 enhancements. Key features include support for stateful precompiled contracts, a new Changes
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
|
skosito
changed the title
skosito
requested review from
brewmaster012,
lumtis,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (13)
zetaclient/chains/evm/signer/signer_test.go (2)
Line range hint
242-266: Consider enhancing test context setup with node account scenarios.The
makeCtxfunction provides a basic test context, but it could be more comprehensive by testing different node account configurations.Consider adding test helper functions to create different node account scenarios:
func makeNodeAccounts(count int) []*observertypes.NodeAccount { accounts := make([]*observertypes.NodeAccount, count) for i := 0; i < count; i++ { accounts[i] = &observertypes.NodeAccount{ Operator: sample.AccAddress(), // Assuming sample package has this helper Status: observertypes.NodeStatus_Active, } } return accounts }Then update the test context setup:
func makeCtx(t *testing.T) context.Context { // ... existing setup ... nodeAccounts := makeNodeAccounts(3) // Create test node accounts err := app.Update( observertypes.Keygen{}, []chains.Chain{chains.BscMainnet, chains.ZetaChainMainnet}, nil, map[int64]*observertypes.ChainParams{ chains.BscMainnet.ChainId: &bscParams, }, "tssPubKey", observertypes.CrosschainFlags{}, nodeAccounts, ) // ... rest of the function ... }
Line range hint
1-266: Consider improving test coverage with additional scenarios.The test file has good structure but could benefit from additional test cases:
- Error scenarios for
TryProcessOutbound:func TestSigner_TryProcessOutbound_Errors(t *testing.T) { tests := []struct { name string setup func(*mocks.EVMRPCClient) wantErr bool }{ { name: "transaction broadcast fails", setup: func(client *mocks.EVMRPCClient) { client.On("SendTransaction", mock.Anything, mock.Anything). Return(errors.New("broadcast failed")) }, wantErr: true, }, // Add more test cases } // ... implement test loop }
- Validation for different chain configurations:
func TestSigner_WithDifferentChains(t *testing.T) { tests := []struct { name string chain chains.Chain wantError bool }{ { name: "unsupported chain", chain: chains.Chain{ChainId: 999999}, wantError: true, }, // Add more test cases } // ... implement test loop }zetaclient/context/app_test.go (1)
92-95: Consider expanding test coverage with multiple node accounts.While the current test is valid, consider enhancing it with multiple node accounts to verify proper handling of various scenarios:
nodeAccounts := []*types.NodeAccount{ sample.NodeAccount(), + sample.NodeAccount(), // Add another node account + sample.NodeAccount(), // And another with different properties }Also applies to: 97-97, 109-109
zetaclient/orchestrator/bootstap_test.go (1)
456-456: Consider adding test cases for node account scenarios.While the
nodeAccountsparameter has been added to themustUpdateAppContextfunction, the test suite could benefit from explicit test cases that verify the behavior of signer and observer maps with different node account configurations.Consider adding test cases that:
- Verify behavior with empty node accounts
- Verify behavior with multiple node accounts
- Verify behavior when node accounts change during runtime
zetaclient/chains/evm/observer/observer_test.go (1)
79-79: Consider adding test cases with non-empty node accounts.The addition of empty node accounts list maintains the existing test functionality. However, since this is part of enhancing node account management, consider adding test cases that verify behavior with populated node accounts to ensure comprehensive coverage.
Example test case addition:
// Add to existing test cases mockNodeAccounts := []*observertypes.NodeAccount{ { Operator: "operator1", Status: observertypes.NodeStatus_Active, }, { Operator: "operator2", Status: observertypes.NodeStatus_Active, }, } err := appContext.Update( observertypes.Keygen{}, []chains.Chain{evmChain, chains.ZetaChainMainnet}, nil, chainParams, "tssPubKey", *sample.CrosschainFlags(), mockNodeAccounts, )go.mod (2)
Line range hint
3-5: Fix Go version mismatch.The module declares
go 1.22.2but uses toolchaingo1.22.5. This mismatch should be resolved by updating the Go version declaration to match the toolchain version.Apply this diff to fix the version mismatch:
-go 1.22.2 +go 1.22.5
370-371: Consider using tagged versions instead of commit hashes.The replace directive uses a specific commit hash which could be volatile and makes it harder to track version changes. Consider using tagged versions if available.
If tagged versions are available, consider applying this diff:
- gitlab.com/thorchain/tss/go-tss => github.com/zeta-chain/go-tss v0.0.0-20241028184352-5558cea583cb + gitlab.com/thorchain/tss/go-tss => github.com/zeta-chain/go-tss v1.0.0 # Replace with appropriate version tagzetaclient/orchestrator/orchestrator_test.go (1)
554-554: Consider using a more explicit empty slice initialization.While passing an empty slice is correct, consider using
make([]*observertypes.NodeAccount, 0)for better clarity and consistency with Go idioms.-[]*observertypes.NodeAccount{}, +make([]*observertypes.NodeAccount, 0),zetaclient/tss/tss_signer.go (4)
189-190: Add documentation for the whitelisted peers parameter.Consider adding documentation that explains:
- The purpose of whitelisting peers
- Expected format and validation rules for peer IDs
- Impact on TSS server behavior
Line range hint
391-392: Fix typo in error messages.Multiple instances of "signuature" should be "signature".
Apply this fix:
-return [][65]byte{}, fmt.Errorf("signuature verification fail") +return [][65]byte{}, fmt.Errorf("signature verification fail")Also applies to: 396-397, 401-402
Line range hint
608-609: Move TestKeysign to a test package.As noted in the TODO comment, this test function should be moved to a dedicated test package. This would:
- Improve code organization
- Separate test code from production code
- Allow for better test coverage tracking
Line range hint
673-700: Enhance error handling in verifySignature.The signature verification function could benefit from more specific error messages and granular error handling:
- Each verification step should return a distinct error message
- Consider using custom error types for different verification failures
Example improvement:
if err != nil { - log.Error().Err(err).Msg("decoding signature R") + log.Error().Err(err).Msg("failed to decode signature R component") return false }changelog.md (1)
27-27: Consider adding more details to the feature entry.The entry "whitelist connection gater" would benefit from additional context explaining its purpose and impact. Consider expanding it to describe what the feature does and why it was added, similar to other feature entries in the changelog.
-* [3028](https://github.com/zeta-chain/node/pull/3028) - whitelist connection gater +* [3028](https://github.com/zeta-chain/node/pull/3028) - Add whitelist connection gater to control and restrict peer connections for enhanced security
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
- changelog.md (1 hunks)
- cmd/zetaclientd/start.go (2 hunks)
- go.mod (1 hunks)
- zetaclient/chains/evm/observer/observer_test.go (1 hunks)
- zetaclient/chains/evm/signer/signer_test.go (1 hunks)
- zetaclient/context/app.go (5 hunks)
- zetaclient/context/app_test.go (8 hunks)
- zetaclient/orchestrator/bootstap_test.go (1 hunks)
- zetaclient/orchestrator/orchestrator_test.go (1 hunks)
- zetaclient/tss/tss_signer.go (2 hunks)
- zetaclient/zetacore/client.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
cmd/zetaclientd/start.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/observer/observer_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/signer_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/context/app.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/context/app_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/orchestrator/bootstap_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/orchestrator/orchestrator_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/tss/tss_signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/zetacore/client.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
zetaclient/orchestrator/orchestrator_test.go (4)
Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-07-04T23:46:38.428Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-10-08T15:34:48.217Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-10-08T15:34:48.217Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-10-08T15:34:47.578Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.
🔇 Additional comments (11)
zetaclient/chains/evm/signer/signer_test.go (1)
263-263: LGTM: Node accounts parameter addition aligns with AppContext changes.The addition of an empty node accounts slice as a parameter to
app.Updatecorrectly reflects the broader changes toAppContextstructure.zetaclient/context/app_test.go (1)
72-72: LGTM: Good initialization check.The assertion properly verifies the initial empty state of nodeAccounts, maintaining consistency with other initialization checks.
zetaclient/context/app.go (2)
40-42: LGTM: Field addition follows Go best practicesThe new field is well-documented and appropriately uses a slice of pointers for reference types.
57-57: LGTM: Proper initialization with empty sliceUsing an empty slice rather than nil is the correct approach for collections in Go.
zetaclient/zetacore/client.go (1)
376-379: LGTM: Node accounts retrieval follows best practices.The implementation demonstrates proper error handling with descriptive context and maintains consistency with the existing codebase pattern.
zetaclient/orchestrator/bootstap_test.go (1)
Line range hint
1-494: Test suite demonstrates excellent coverage and structure.The test cases effectively validate:
- Dynamic chain management (addition, removal, re-enabling)
- Multiple chain type support (EVM, Bitcoin, Solana, TON)
- Edge cases and error scenarios
The well-structured arrange-act-assert pattern and comprehensive helper functions make the tests maintainable and readable.
cmd/zetaclientd/start.go (1)
221-221: LGTM: TSS server setup with whitelisted peers.The implementation correctly integrates the whitelisted peers into the TSS server setup, enhancing security through connection gating.
zetaclient/orchestrator/orchestrator_test.go (2)
Line range hint
32-554: Test coverage looks comprehensive!The test suite effectively covers:
- Different chain types (EVM, Bitcoin, Solana)
- Error cases and edge scenarios
- Chain parameter updates
- Rate limiter functionality
Line range hint
1-554: Code organization follows Go best practices.The test file demonstrates excellent organization with:
- Well-structured table-driven tests
- Clear separation of test setup and assertions
- Reusable helper functions
- Comprehensive mock implementations
zetaclient/tss/tss_signer.go (1)
151-152: LGTM: Clean function signature update.The addition of
whitelistedPeers []gopeer.IDparameter is well-structured and maintains backward compatibility.changelog.md (1)
Line range hint
1-1000: LGTM! Well-structured changelog.The changelog follows a consistent and clear format with:
- Proper version numbering
- Categorized changes (Features, Breaking Changes, Refactor, etc.)
- PR links for traceability
- Detailed descriptions for significant changes
|
Let's make sure we use the final merged version of zeta-chain/go-tss#31 in go.mod |
skosito
added
TSS_MIGRATION_TESTS
for first step i added tss migration test label, will follow up on results and check what is proper way to test |
Yes, the TSS migration test does a new keygen , using the node accounts. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* bump go tss to remove dht * add whitelist fields * disable whitelist for localnet * bump go-tss * resolve whitelisted peers wip * dont disable whitelist in e2e tests * cleanup whitelist fields from config and fix e2e tests * bump go-tss * cleanup * bump go tss * use node accounts to get whitelisted peers * bump go tss * changelog * fix unit test * bump go tss * remove usage of pointers in node accounts * fix unit test * revert back to using keygen for whitelist peers --------- Co-authored-by: Alex Gartner <[email protected]>
* feat: whitelist connection gater (#3028) * bump go tss to remove dht * add whitelist fields * disable whitelist for localnet * bump go-tss * resolve whitelisted peers wip * dont disable whitelist in e2e tests * cleanup whitelist fields from config and fix e2e tests * bump go-tss * cleanup * bump go tss * use node accounts to get whitelisted peers * bump go tss * changelog * fix unit test * bump go tss * remove usage of pointers in node accounts * fix unit test * revert back to using keygen for whitelist peers --------- Co-authored-by: Alex Gartner <[email protected]> * fix: replace DHT with private peer discovery (#3041) * import go-tss lib that removes DHT * replace DHT with authenticated discovery * use JSON serialization; add metric * add new telemetry: 8123/connectedpeers; fix deadlock in a few other 8123 handlers * use squashed go-tss commit * clean up interface * address review comments * remove whiteliste peers * fmt * remove rendezvous * use merged go-tss connection gater * use latest go-tss from PR#34 * use merged #34 in go-tss lib * add ping RTT to telemetry * changelog * make linter happy * pingrtt * finer resolution on pingrtt time (milliseconds => nanoseconds) * removed comments * bump go-tss to the merged commit in master branch * revert back to the go-tss commit until the PR commit is merged. --------- Co-authored-by: pwu <[email protected]> * skip depositor fee calculation on irrelevant txs * update changelog --------- Co-authored-by: skosito <[email protected]> Co-authored-by: brewmaster012 <[email protected]> Co-authored-by: pwu <[email protected]> Co-authored-by: Charlie Chen <[email protected]>
Description
go-tss PR zeta-chain/go-tss#31
How Has This Been Tested?
Summary by CodeRabbit
Release Notes for Version 21.0.0
New Features
zetacoredRPC package.whitelist connection gaterfor improved peer management.Bug Fixes
Tests
AppContextand orchestrator functionalities.Documentation