refactor(zetaclient): remove redundant getters/setters #3268
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 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant refactoring across multiple files in the Changes
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
|
swift1337
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3268 +/- ##
===========================================
- Coverage 61.81% 61.72% -0.09%
===========================================
Files 432 432
Lines 30875 30809 -66
===========================================
- Hits 19084 19016 -68
- Misses 10930 10934 +4
+ Partials 861 859 -2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (33)
cmd/zetaclientd/inbound.go (3)
98-107: Enhance type assertion error handlingWhen asserting the observer type, the error messages could be more informative by including the expected and actual types. This will aid in debugging if an incorrect observer type is retrieved.
Modify the error handling as follows:
observer, ok := observers[chainID] if !ok { return fmt.Errorf("observer not found for EVM chain %d", chain.ID()) } -evmObserver, ok := observer.(*evmobserver.Observer) +evmObserver, ok := observer.(evmobserver.EVMObserver) if !ok { - return fmt.Errorf("observer is not evm observer for chain %d", chain.ID()) + return fmt.Errorf("expected EVMObserver for chain %d, got %T", chain.ID(), observer) }Ensure that
EVMObserveris the correct interface or type thatobservershould implement.
159-161: Consistent observer retrieval error messagesThe error message for missing observers in the Bitcoin chain should be consistent with the EVM chain error handling.
Update the error message for clarity:
if !ok { - return fmt.Errorf("observer not found for btc chain %d", chainID) + return fmt.Errorf("observer not found for Bitcoin chain %d", chain.ID()) }
164-166: Improve type assertion failure message for Bitcoin observerSimilar to the EVM observer, enhance the error message when the observer is not of the expected type.
Modify the error handling as follows:
btcObserver, ok := observer.(*btcobserver.Observer) if !ok { - return fmt.Errorf("observer is not btc observer for chain %d", chainID) + return fmt.Errorf("expected BTCObserver for chain %d, got %T", chain.ID(), observer) }zetaclient/chains/base/signer_test.go (1)
21-21: Ensure comprehensive test coverage forSignerWith the removal of the
TelemetryServer, consider adding tests that verify the behavior of the updatedSignerconstructor and its methods to maintain high test coverage.Implement additional test cases to cover the new logger initialization and any other logic changes introduced by the refactoring.
zetaclient/chains/evm/observer/observer_gas_test.go (6)
27-27: Handle potential errors from mock methodsWhen setting up mocks with
On(...).Return(...), consider simulating possible errors to test the observer's error handling logic.Add test cases where
observer.evmClient.On("HeaderByNumber", ...).Return(nil, error)to ensure the observer can gracefully handle RPC errors.
30-31: Consolidate mock return valuesSince
SuggestGasPriceandSuggestGasTipCapare essential for computing gas metrics, ensure that any potential anomalies in their return values are properly handled in tests.Consider adding assertions to validate that the observer correctly processes zero or unusually high gas prices.
60-60: Reuse test setup for consistencySince both sub-tests share similar setups, consider extracting common initialization code to improve readability and reduce duplication.
Refactor the test as follows:
observer := newTestSuite(t) +setupMockRPCResponses(observer, baseFee, gasPrice, priorityFee)Define
setupMockRPCResponsesto configure the RPC responses based on the test scenario.
63-64: Clarify the context for EIP-1559 support in commentsUpdate the comment to reflect that a non-nil
BaseFeeindicates EIP-1559 support, enhancing code clarity.Modify the comment:
// Given 1 gwei baseFee from RPC, indicating EIP-1559 support
67-68: Ensure realistic gas price simulationsWhen mocking
SuggestGasPriceandSuggestGasTipCap, use values that closely resemble real-world scenarios to make tests more robust.Adjust the mocked values if necessary to reflect typical gas prices during different network conditions.
78-78: Validate the response fromPostVoteGasPriceCurrently, the test assumes that
PostVoteGasPricereturns a transaction hash. Consider verifying this return value to ensure the observer handles the response correctly.Add an assertion to check the returned transaction hash:
assert.Equal(t, "0xABC123...", txHash)Adjust the code accordingly to capture the return value.
zetaclient/chains/base/signer.go (4)
6-7: Organize imports according to Go conventionsGroup standard library imports separately from third-party and local package imports to enhance readability.
Reorder the imports:
import ( + "sync" - "github.com/rs/zerolog" - + "github.com/rs/zerolog" + "github.com/zeta-chain/node/pkg/chains" "github.com/zeta-chain/node/zetaclient/chains/interfaces" "github.com/zeta-chain/node/zetaclient/logs" )
34-41: Simplify logger initializationDefining the
withLogFieldsclosure insideNewSignermay reduce readability. Consider extracting it into a separate function or method.Extract
withLogFields:func withLogFields(log zerolog.Logger, chainID int64) zerolog.Logger { return log.With(). Int64(logs.FieldChain, chainID). Str(logs.FieldModule, "signer"). Logger() }Update
NewSigner:func NewSigner(chain chains.Chain, tss interfaces.TSSSigner, logger Logger) *Signer { - withLogFields := func(log zerolog.Logger) zerolog.Logger { - return log.With(). - Int64(logs.FieldChain, chain.ChainId). - Str(logs.FieldModule, "signer"). - Logger() - } return &Signer{ chain: chain, tss: tss, outboundBeingReported: make(map[string]bool), logger: Logger{ - Std: withLogFields(logger.Std), - Compliance: withLogFields(logger.Compliance), + Std: withLogFields(logger.Std, chain.ChainId), + Compliance: withLogFields(logger.Compliance, chain.ChainId), }, } }
43-48: Initialize maps with proper capacity if possibleIf the expected number of outbound transactions is known or can be estimated, initializing
outboundBeingReportedwith a capacity can improve performance.For example:
outboundBeingReported: make(map[string]bool), +// If expected size is known +// outboundBeingReported: make(map[string]bool, expectedSize),Replace
expectedSizewith an appropriate value if available.
58-58: Exported method comments should follow Go conventionsEnsure that the comment for
TSS()begins with the method name for consistency with Go documentation standards.Modify the comment:
-// TSS returns the tss signer for the signer. +// TSS returns the TSS signer associated with the Signer.zetaclient/chains/evm/signer/sign_test.go (1)
17-17: LGTM: Improved test organization with centralized setup.The refactoring enhances maintainability by:
- Centralizing test setup through
newTestSuite- Reducing code duplication
- Maintaining consistent access to TSS functionality
Consider adding a comment in the test suite initialization to document the purpose and structure of the centralized setup.
Also applies to: 32-32, 36-36, 42-42, 51-51, 77-77, 84-84, 99-99, 113-113, 121-121, 134-134, 148-148, 152-152, 156-156, 169-169, 183-183, 190-190, 203-203, 217-217, 226-226
zetaclient/chains/evm/observer/observer.go (2)
246-247: Add test coverage for TransactionReceipt methodThe implementation looks correct, but test coverage is missing according to the static analysis report.
Would you like me to help generate test cases for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 246-247: zetaclient/chains/evm/observer/observer.go#L246-L247
Added lines #L246 - L247 were not covered by tests
246-247: Consider adding context timeout handlingWhile the implementation is correct, consider adding context timeout handling for better resilience in production environments.
func (ob *Observer) TransactionReceipt(ctx context.Context, hash ethcommon.Hash) (*ethtypes.Receipt, error) { + // Add timeout to prevent hanging in case of network issues + ctx, cancel := context.WithTimeout(ctx, ob.Chain().Timeout) + defer cancel() return ob.evmClient.TransactionReceipt(ctx, hash) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 246-247: zetaclient/chains/evm/observer/observer.go#L246-L247
Added lines #L246 - L247 were not covered by testszetaclient/chains/evm/observer/observer_test.go (2)
251-255: Consider adding test coverage for cache miss scenariosWhile the cache hit scenario is well tested, consider adding test cases for:
- Cache miss with successful RPC fallback
- Cache miss with RPC failure
Also applies to: 258-265
390-403: Consider enhancing test suite configuration optionsThe configuration structure allows for chain customization, which is good. Consider adding more configuration options for:
- Custom block numbers
- Mock response configurations
- Network conditions (delays, timeouts)
zetaclient/chains/bitcoin/signer/signer.go (1)
Line range hint
95-98: Consider adding block cache size validation.While the error handling is good, consider adding validation for the block cache size parameter to ensure it's within reasonable bounds.
+ if blockCacheSize <= 0 { + return nil, fmt.Errorf("block cache size must be positive, got %d", blockCacheSize) + } blockCache, err := lru.New(blockCacheSize)zetaclient/chains/base/observer_test.go (4)
35-40: Add documentation for the testSuite structConsider adding documentation to describe the purpose of the testSuite struct and its fields, following Go's documentation conventions.
+// testSuite encapsulates the observer and its dependencies for testing type testSuite struct { *base.Observer db *db.DB tss *mocks.TSS zetacore *mocks.ZetacoreClient }
Line range hint
188-196: Enhance logger test assertionsThe logger test case only verifies that logs can be printed but doesn't assert the output. Consider using a buffer to capture and verify log output.
t.Run("should be able to get logger", func(t *testing.T) { ob := newTestSuite(t, chain, defaultAlertLatency) logger := ob.Logger() + var buf bytes.Buffer + logger.Chain.Output(&buf) // should be able to print log logger.Chain.Info().Msg("print chain log") - logger.Inbound.Info().Msg("print inbound log") - logger.Outbound.Info().Msg("print outbound log") - logger.GasPrice.Info().Msg("print gasprice log") - logger.Headers.Info().Msg("print headers log") - logger.Compliance.Info().Msg("print compliance log") + require.Contains(t, buf.String(), "print chain log") })
202-204: Define test chain ID as a constantThe hardcoded chain ID (123123123) should be defined as a named constant to improve maintainability and make its purpose clear.
+const testUnknownBTCChainID = 123123123 + btcSomething := chains.BitcoinMainnet -btcSomething.ChainId = 123123123 +btcSomething.ChainId = testUnknownBTCChainID
Line range hint
477-489: Clean up environment variables after testThe test modifies environment variables but doesn't restore them afterwards. This could affect other tests.
t.Run("should overwrite last tx scanned if env var is set", func(t *testing.T) { + // Save original env var value + origEnvVar := os.Getenv(envvar) + defer func() { + if origEnvVar != "" { + os.Setenv(envvar, origEnvVar) + } else { + os.Unsetenv(envvar) + } + }() + // create observer and open db ob := newTestSuite(t, chain, defaultAlertLatency) // ... rest of the test })zetaclient/chains/evm/observer/inbound_test.go (1)
449-452: Define mock setup function typesConsider defining types for the mock setup functions to improve code readability and maintainability.
+type mockEVMClientFunc func(*mocks.EVMRPCClient) +type mockJSONClientFunc func(*mocks.MockJSONRPCClient) + tests := []struct { - name string - mockEVMClient func(m *mocks.EVMRPCClient) - mockJSONClient func(m *mocks.MockJSONRPCClient) - errMsg string + name string + mockEVMClient mockEVMClientFunc + mockJSONClient mockJSONClientFunc + errMsg string }{zetaclient/chains/evm/observer/outbound_test.go (1)
421-430: Consider consolidating mock setup in newTestSuite.The mock setup for the EVM client could be moved into the
newTestSuitefunction to maintain consistency and reduce duplication across test cases.zetaclient/chains/bitcoin/observer/inbound_test.go (3)
158-159: Add documentation for the test suite initialization.Consider adding documentation to explain the test suite's structure, purpose, and initialization requirements. This will help other developers understand how to properly set up and extend test cases.
Add a comment block above the initialization:
+// newTestSuite creates a new test suite with a mock observer and zetacore client. +// The test suite provides a standardized way to test Bitcoin observer functionality +// with consistent mock behaviors and initialization patterns. ob := newTestSuite(t, chain) ob.zetacore.WithKeys(&keys.Keys{}).WithZetaChain()
Line range hint
432-438: Enhance error assertions in test cases.The error assertions could be more specific to ensure exact error conditions are being tested. Consider using
errors.Isor more specific error message checks.Example improvement:
-require.Error(t, err) -require.ErrorContains(t, err, "invalid block height") +var expectedErr = &bitcoin.InvalidBlockHeightError{} +require.True(t, errors.Is(err, expectedErr)) +require.Equal(t, "block height 0 is invalid", err.Error())
Line range hint
41-42: Consolidate test data paths and values.Consider moving hardcoded test data paths and values to constants or a dedicated test configuration structure. This would make test maintenance easier and reduce duplication.
Example improvement:
+const ( + testBlockHeight = 828440 + testDataPathBTC = "block_trimmed_8332_828440.json" + testDataPathMempoolBTC = "block_mempool.space_8332_828440.json" +) -testutils.LoadObjectFromJSONFile( - t, - &blockVb, - path.Join(TestDataDir, testutils.TestDataPathBTC, "block_trimmed_8332_828440.json"), -) +testutils.LoadObjectFromJSONFile( + t, + &blockVb, + path.Join(TestDataDir, testutils.TestDataPathBTC, testDataPathBTC), +)Also applies to: 46-47
zetaclient/chains/bitcoin/observer/outbound_test.go (2)
113-113: Consider using test helper for observer creation.The repeated initialization of the observer with
MockBTCObserverMainnet(t, nil)across multiple test functions suggests an opportunity for a test helper function to reduce code duplication.+func createDefaultObserver(t *testing.T) *Observer { + return MockBTCObserverMainnet(t, nil) +} func TestCheckTSSVout(t *testing.T) { chain := chains.BitcoinMainnet chainID := chain.ChainId nonce := uint64(148) - ob := MockBTCObserverMainnet(t, nil) + ob := createDefaultObserver(t)Also applies to: 195-195
Line range hint
4-4: Consider adding test coverage metrics.The test file would benefit from a coverage comment indicating the percentage of code coverage achieved, helping maintain high test coverage standards.
Add a comment like:
// Test coverage: 95.2% of zetaclient/chains/bitcoin/observer/outbound.gozetaclient/chains/bitcoin/observer/observer_test.go (2)
300-306: Add documentation to testSuite struct.Consider adding documentation to explain the purpose of each field in the testSuite struct.
+// testSuite encapsulates the Bitcoin observer and its dependencies for testing. type testSuite struct { + // Observer is the main Bitcoin observer instance being tested *observer.Observer + // client is a mock Bitcoin RPC client for simulating blockchain interactions client *mocks.BTCRPCClient + // zetacore is a mock Zetacore client for simulating core interactions zetacore *mocks.ZetacoreClient + // db is an in-memory database for testing db *db.DB }
308-342: Add documentation to newTestSuite function.Consider adding comments to explain the initialization steps.
+// newTestSuite creates a new test suite with a Bitcoin observer and mock dependencies. +// It initializes an in-memory database and sets up mock clients for testing. func newTestSuite(t *testing.T, chain chains.Chain) *testSuite { require.True(t, chain.IsBitcoinChain()) + // Initialize chain parameters with mock values chainParams := mocks.MockChainParams(chain.ChainId, 10) + // Set up mock Bitcoin RPC client client := mocks.NewBTCRPCClient(t) client.On("GetBlockCount").Return(int64(100), nil).Maybe() + // Set up mock Zetacore client zetacore := mocks.NewZetacoreClient(t)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (30)
cmd/zetaclientd/inbound.go(5 hunks)cmd/zetaclientd/start.go(1 hunks)zetaclient/chains/base/observer.go(3 hunks)zetaclient/chains/base/observer_test.go(27 hunks)zetaclient/chains/base/signer.go(3 hunks)zetaclient/chains/base/signer_test.go(1 hunks)zetaclient/chains/bitcoin/observer/event_test.go(3 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(1 hunks)zetaclient/chains/bitcoin/observer/observer.go(0 hunks)zetaclient/chains/bitcoin/observer/observer_test.go(8 hunks)zetaclient/chains/bitcoin/observer/outbound_test.go(5 hunks)zetaclient/chains/bitcoin/signer/signer.go(1 hunks)zetaclient/chains/bitcoin/signer/signer_test.go(0 hunks)zetaclient/chains/evm/observer/inbound_test.go(20 hunks)zetaclient/chains/evm/observer/observer.go(1 hunks)zetaclient/chains/evm/observer/observer_gas_test.go(4 hunks)zetaclient/chains/evm/observer/observer_test.go(6 hunks)zetaclient/chains/evm/observer/outbound_test.go(7 hunks)zetaclient/chains/evm/signer/sign_test.go(13 hunks)zetaclient/chains/evm/signer/signer.go(1 hunks)zetaclient/chains/evm/signer/signer_admin_test.go(17 hunks)zetaclient/chains/evm/signer/signer_test.go(6 hunks)zetaclient/chains/solana/observer/observer.go(0 hunks)zetaclient/chains/solana/signer/signer.go(1 hunks)zetaclient/chains/solana/signer/signer_test.go(4 hunks)zetaclient/chains/ton/observer/observer_test.go(0 hunks)zetaclient/chains/ton/signer/signer_test.go(1 hunks)zetaclient/orchestrator/bootstap_test.go(7 hunks)zetaclient/orchestrator/bootstrap.go(4 hunks)zetaclient/orchestrator/orchestrator.go(1 hunks)
💤 Files with no reviewable changes (4)
- zetaclient/chains/ton/observer/observer_test.go
- zetaclient/chains/bitcoin/signer/signer_test.go
- zetaclient/chains/solana/observer/observer.go
- zetaclient/chains/bitcoin/observer/observer.go
👮 Files not reviewed due to content moderation or server errors (2)
- zetaclient/chains/evm/signer/signer_test.go
- zetaclient/chains/evm/signer/signer_admin_test.go
🧰 Additional context used
📓 Path-based instructions (26)
zetaclient/chains/base/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/event_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/orchestrator/orchestrator.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/sign_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetaclientd/inbound.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_admin_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
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_gas_test.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.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/outbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/base/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/base/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/base/signer.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/chains/bitcoin/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/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.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/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/bootstrap.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
zetaclient/chains/bitcoin/observer/inbound_test.go (2)
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.
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:366-371
Timestamp: 2024-11-12T13:20:12.658Z
Learning: The `bitcoin.CalcDepositorFee` function is covered by live tests.
🪛 GitHub Check: codecov/patch
zetaclient/orchestrator/orchestrator.go
[warning] 776-776: zetaclient/orchestrator/orchestrator.go#L776
Added line #L776 was not covered by tests
zetaclient/chains/evm/observer/observer.go
[warning] 246-247: zetaclient/chains/evm/observer/observer.go#L246-L247
Added lines #L246 - L247 were not covered by tests
zetaclient/orchestrator/bootstrap.go
[warning] 179-179: zetaclient/orchestrator/bootstrap.go#L179
Added line #L179 was not covered by tests
[warning] 200-200: zetaclient/orchestrator/bootstrap.go#L200
Added line #L200 was not covered by tests
🔇 Additional comments (24)
zetaclient/chains/evm/observer/observer_gas_test.go (2)
24-24: Verify mock initialization in newTestSuite
Ensure that newTestSuite(t) correctly initializes all necessary mocks and dependencies for the observer to prevent unexpected test failures.
Confirm that newTestSuite sets up observer.evmClient and observer.zetacore with the expected behaviors.
41-41: Check the correctness of the mock collector
Verify that the mock collector function captures the arguments accurately, especially with respect to the order and types of the parameters.
Ensure that args.Get(2) and args.Get(3) correspond to gasPrice and priorityFee respectively, and that they are of type uint64.
zetaclient/chains/solana/signer/signer_test.go (3)
92-92: LGTM: Constructor call updated correctly.
The removal of the telemetry server parameter aligns with the PR's objective to eliminate redundant parameters.
113-113: LGTM: Consistent parameter update.
The change maintains consistency with the refactoring pattern across the codebase.
162-162: LGTM: Parameter updates maintain test integrity.
Both instances of NewSigner have been correctly updated, preserving the test's functionality while removing unnecessary parameters.
Also applies to: 177-177
zetaclient/chains/ton/signer/signer_test.go (1)
169-169: LGTM: Test suite initialization updated correctly.
The base signer initialization has been properly updated to align with the new parameter structure.
cmd/zetaclientd/start.go (1)
200-200: LGTM: Simplified CreateSignerMap interface
The removal of the telemetryServer parameter aligns with the PR's objective of reducing unnecessary dependencies and simplifying interfaces.
zetaclient/chains/solana/signer/signer.go (1)
59-59: LGTM: Simplified NewSigner interface
The removal of telemetry server dependency from base.NewSigner call is consistent with the codebase-wide refactoring effort to minimize dependencies.
zetaclient/chains/evm/observer/observer_test.go (2)
209-210: LGTM: Test suite refactoring improves maintainability
The introduction of newTestSuite and its usage across test cases improves code organization and reduces setup duplication. The mock setup for the EVM client is clear and follows testing best practices.
Also applies to: 223-223, 233-233, 239-240
379-388: LGTM: Well-structured test suite type
The testSuite struct provides a clean interface for managing test dependencies and makes the test setup more maintainable.
zetaclient/chains/bitcoin/observer/event_test.go (1)
309-309: LGTM: Consistent test suite adoption
The changes consistently apply the new test suite pattern across different test cases, improving code maintainability.
Also applies to: 357-358, 397-398
zetaclient/orchestrator/bootstrap.go (2)
45-45: LGTM: Simplified signer map synchronization
The removal of the telemetry server parameter simplifies the interface while maintaining functionality.
148-148: LGTM: Clean Bitcoin signer initialization
The Bitcoin signer initialization is clean and follows the pattern of other chain implementations.
zetaclient/chains/bitcoin/signer/signer.go (2)
69-69: LGTM: Clean function signature refactoring.
The removal of the telemetry server parameter simplifies the interface while maintaining functionality.
Line range hint 100-115: LGTM: Clean struct initialization.
The initialization is well-structured with all required fields properly set.
zetaclient/orchestrator/bootstap_test.go (1)
66-66: LGTM: Consistent test refactoring.
The removal of telemetry server parameter from test functions maintains consistency with the production code changes.
Also applies to: 84-84, 103-103, 124-124, 144-144, 166-166, 183-183
zetaclient/chains/base/observer.go (3)
95-98: LGTM: Clean block cache initialization.
The error handling for block cache creation is clear and appropriate.
Line range hint 100-115: LGTM: Clean observer initialization.
The observer initialization is well-structured with proper field assignments.
467-486: LGTM: Well-structured logger initialization.
The new logger initialization function is clean and modular, with proper field assignments for each logger type.
zetaclient/orchestrator/orchestrator.go (1)
776-776: Add test coverage for the modified syncSignerMap call.
The removal of the telemetry server parameter from syncSignerMap is not covered by tests. Consider adding test cases to verify that the signer map synchronization still works correctly without the telemetry server.
✅ Verification successful
Let me check the actual test coverage by examining the test file content.
Test coverage for syncSignerMap is adequate
The function is well-tested in bootstap_test.go with multiple test cases covering different scenarios:
- Adding new signers (asserting added = 1, removed = 0)
- Removing signers (asserting added = 0, removed = 1)
- No changes (asserting added = 0, removed = 0)
The removal of the telemetry server parameter was a safe refactoring as the parameter was not used in the function's implementation, as verified by examining the complete function source.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage and usage of syncSignerMap
# Test: Look for test files that should cover this change
rg -l "syncSignerMap" --type go --glob "*_test.go"
Length of output: 91
Script:
#!/bin/bash
# Check the content of the test file to see how syncSignerMap is used
rg "syncSignerMap" zetaclient/orchestrator/bootstap_test.go -B 2 -A 5
# Also check the implementation of syncSignerMap to understand the changes
ast-grep --pattern 'func syncSignerMap($$$) ($$$) {
$$$
}'
Length of output: 12194
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 776-776: zetaclient/orchestrator/orchestrator.go#L776
Added line #L776 was not covered by tests
zetaclient/chains/evm/observer/outbound_test.go (1)
441-446: LGTM! Test assertions are comprehensive.
The test assertions thoroughly verify both the transaction and receipt properties.
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Line range hint 1-1000: Overall test implementation looks solid.
The test coverage is comprehensive, covering various Bitcoin address types, error scenarios, and edge cases. The changes align well with the PR objectives of refactoring and improving the test structure.
zetaclient/chains/bitcoin/observer/outbound_test.go (2)
26-33: LGTM: Well-structured function modification with proper nil handling.
The addition of the tss parameter with a nil check and default value implementation follows good testing practices by allowing both explicit TSS signer injection and default behavior.
Line range hint 1-466: Excellent test coverage and structure.
The test suite demonstrates comprehensive coverage of Bitcoin transaction handling:
- Thorough validation of TSS vout scenarios
- Extensive UTXO selection test cases
- Detailed UTXO consolidation scenarios
- Clear test case descriptions
- Proper error validation
lumtis
added
UPGRADE_LIGHT_TESTS
Scope
Changes
With*)testSuite{}Example:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests