refactor(zetaclient)!: improve AppContext

[swift1337]

Description

This PR converges chains, supportedChains, chainParams, enabledChains, and additionalChains into a single zctx.Chain in zetaclient with a dead-simple API.

The main idea is that regardless of the many entities in zetacore, zetaclient should only operate with the notion of just a "chain" and have ONLY supported and enabled chains in zctx.Context. That constraint significantly simplifies the code logic.

It also drops block header verification from zetaclient. cc @lumtis

The diff is huge due to project-wide refactoring, but the main changes are located in zetaclient/context/chain.go and zetaclient/context/app.go.

Partially closes: #2519
Closes: #2419

How Has This Been Tested?

• Tested CCTX in localnet
• Tested in development environment
• Go unit tests
• Go integration tests
• Tested via GitHub Actions

Summary by CodeRabbit

• New Features

  • Enhanced application context management for improved state handling.
  • Introduced a new Map functionality to enhance code reusability.
  • Added Empty() method in the EVMConfig struct for additional functionality.
  • New ChainRegistry for managing supported blockchain networks.

• Bug Fixes

  • Improved error handling and clarity in various functions.

• Documentation

  • Updated changelog to reflect significant changes in the application context and other components.

• Tests

  • Expanded and refined test coverage across various modules for better validation of changes.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Walkthrough

This pull request unifies the lifecycle management of chain parameters, signers, and observers within the AppContext class, replacing multiple specific configurations with a more generic and scalable approach. It introduces a ChainRegistry to streamline the registration, provisioning, and management of different blockchain types, enhancing the maintainability and flexibility of the application.

Changes

Files	Change Summary
zetaclient/context/app.go, zetaclient/context/chain.go, zetaclient/context/chain_test.go	Introduced a ChainRegistry for managing blockchain networks and restructured AppContext for better scalability.
zetaclient/orchestrator/orchestrator.go, zetaclient/orchestrator/orchestrator_test.go, zetaclient/orchestrator/bootstrap.go	Enhanced the Orchestrator’s handling of chain parameters and unified function signatures for better clarity.
zetaclient/zetacore/client.go, zetaclient/zetacore/client_query_observer.go, zetaclient/zetacore/client_worker.go	Updated methods for managing application context with improved error handling and streamlined parameters.
zetaclient/zetacore/client_query_test.go, zetaclient/testutils/mocks/chain_params.go	Modified test cases and mock functions to reflect changes in method signatures and error handling improvements.

Assessment against linked issues

Objective	Addressed	Explanation
Unify chainParams, signer, and observers lifecycle (#[2519])	✅
Simplify management of chain types, observers, and signers (#[2519])	✅
Improve error handling and parameter management in application context (#[2519])	✅
Reduce hardcoded instances of chain parameters (#[2519])	❌	Some hardcoded instances remain in legacy files.
Enhance test coverage for unified lifecycle management (#[2519])	❓	Uncertainty exists regarding complete test coverage.

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 15

Outside diff range, codebase verification and nitpick comments (23)

pkg/slices/slices.go (2)

1-1: Use a more descriptive package name.

The package name slices is generic. Consider using a more descriptive name to better convey the purpose of the package.

---

3-3: Consider adding a more detailed function comment.

While the current comment is clear, providing an example usage could enhance understanding.

// Map applies a function to each item in a slice and returns a new slice with the results.
// Example:
//  input := []int{1, 2, 3}
//  output := Map(input, func(i int) int { return i * 2 })
//  // output is []int{2, 4, 6}

zetaclient/testutils/mocks/chain_params.go (1)

16-16: Consider using a more descriptive constant name.

The constant zeroAddress is clear, but a more descriptive name like defaultZeroAddress could enhance readability.

const defaultZeroAddress = "0x0000000000000000000000000000000000000000"

zetaclient/context/chain.go (1)

15-24: Ensure thread safety with mutex usage.

The ChainRegistry struct uses a mutex for thread safety. The access to chains and additionalChains is properly synchronized with the mutex in the Set, SetAdditionalChains, Delete, and ChainIDs methods. However, the Get, All, and Has methods access chains without acquiring a lock.

• Get method: Add a mutex lock/unlock around the access to chains.
• All method: Add a mutex lock/unlock around the access to chains.
• Has method: Add a mutex lock/unlock around the access to chains.

Please ensure these changes are made to maintain thread safety.

Analysis chain

Ensure thread safety with mutex usage.

The ChainRegistry struct uses a mutex for thread safety. Ensure that all access to chains and additionalChains is properly synchronized.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all access to `chains` and `additionalChains` in `ChainRegistry` is synchronized with the mutex.

# Test: Search for the usage of `chains` and `additionalChains`. Expect: Mutex lock/unlock around all accesses.
rg --type go '\.chains|\.additionalChains'

Length of output: 3078

---

Script:

#!/bin/bash
# Description: Verify that all access to `chains` and `additionalChains` in `ChainRegistry` is synchronized with the mutex.

# Test: Search for mutex lock/unlock operations around the identified lines in `zetaclient/context/chain.go`.
rg --type go -A 5 -B 5 'chains|additionalChains' zetaclient/context/chain.go

Length of output: 3871

zetaclient/context/app_test.go (1)

1-1: Ensure consistent package naming conventions.

The package name should ideally reflect the purpose of the tests. Consider renaming it to context_test to maintain consistency with Go testing conventions.

- package context
+ package context_test

zetaclient/orchestrator/bootstrap.go (2)

Line range hint 85-125: Improve error handling and logging for signer creation.

Ensure that all potential error scenarios are logged and handled gracefully to aid debugging and maintain robustness.

switch {
case chain.IsEVM():
	// existing code
	if err != nil {
		logger.Std.Error().Err(err).Msgf("Unable to construct signer for EVM chain %d", chainID)
		continue
	}
	addSigner(chainID, signer)
case chain.IsUTXO():
	// existing code
	if err != nil {
		logger.Std.Error().Err(err).Msgf("Unable to construct signer for UTXO chain %d", chainID)
		continue
	}
	addSigner(chainID, signer)
default:
	logger.Std.Warn().Msgf("Unsupported chain type for chain %d", chainID)
}

---

Line range hint 211-334: Enhance observer creation with detailed error logging.

Ensure that all potential error scenarios during observer creation are logged with sufficient detail to aid debugging.

switch {
case chain.IsEVM():
	// existing code
	if err != nil {
		logger.Std.Error().Err(err).Str("rpc.endpoint", cfg.Endpoint).Msgf("Unable to dial EVM RPC")
		continue
	}
	addObserver(chainID, observer)
case chain.IsUTXO():
	// existing code
	if err != nil {
		logger.Std.Error().Err(err).Msgf("Unable to create RPC client for BTC chain %d", chainID)
		continue
	}
	addObserver(chainID, btcObserver)
case chain.IsSolana():
	// existing code
	if err != nil {
		logger.Std.Error().Err(err).Msgf("Unable to open database for SOL chain %d", chainID)
		continue
	}
	addObserver(chainID, solObserver)
default:
	logger.Std.Warn().Msgf("Unsupported chain type for chain %d", chainID)
}

zetaclient/zetacore/client.go (3)

347-347: Improve error message consistency.

The error message "unable to get zetablock height" should be consistent with the format used in other error messages in the method.

- return errors.Wrap(err, "unable to get zetablock height")
+ return errors.Wrap(err, "unable to get zetablock height from zetacore")

---

352-352: Enhance error message clarity.

The error message "unable to get upgrade plan" should provide more context about the failure.

- return errors.Wrap(err, "unable to get upgrade plan")
+ return errors.Wrap(err, "unable to get upgrade plan from zetacore")

---

366-366: Consider logging the stop action.

Logging the stop action can provide better traceability and debugging information.

- c.Stop()
+ c.logger.Info().Msg("Stopping ZetaClient due to upgrade plan")
+ c.Stop()

zetaclient/chains/evm/observer/observer.go (6)

Line range hint 1-13:
Remove unused imports.

The imports for rlp and proofs packages are no longer necessary after the removal of the postBlockHeader function.

- "github.com/zeta-chain/zetacore/zetaclient/proofs"
- "github.com/ethereum/go-ethereum/rlp"

---

Line range hint 1-13:
Add package-level documentation.

Consider adding documentation at the package level to describe the purpose and functionality of the observer package.

// Package observer implements the EVM chain observer, responsible for monitoring and interacting with EVM-based blockchains.
package observer

---

Line range hint 15-41:
Consider adding comments to struct fields.

Adding comments to the fields of the Observer struct can improve readability and maintainability.

// Observer is the observer for EVM chains
type Observer struct {
	// base.Observer implements the base chain observer
	base.Observer

	// priorityFeeConfig is the configuration for priority fee
	priorityFeeConfig

	// evmClient is the EVM client for the observed chain
	evmClient interfaces.EVMRPCClient

	// evmJSONRPC is the EVM JSON RPC client for the observed chain
	evmJSONRPC interfaces.EVMJSONRPCClient

	// outboundPendingTransactions is the map to index pending transactions by hash
	outboundPendingTransactions map[string]*ethtypes.Transaction

	// outboundConfirmedReceipts is the map to index confirmed receipts by hash
	outboundConfirmedReceipts map[string]*ethtypes.Receipt

	// outboundConfirmedTransactions is the map to index confirmed transactions by hash
	outboundConfirmedTransactions map[string]*ethtypes.Transaction
}

---

Line range hint 67-77:
Consider adding comments to method parameters.

Adding comments to the parameters of the NewObserver method can improve readability and maintainability.

// NewObserver returns a new EVM chain observer
func NewObserver(
	ctx context.Context,
	evmCfg config.EVMConfig, // EVM configuration
	evmClient interfaces.EVMRPCClient, // EVM RPC client
	chainParams observertypes.ChainParams, // Chain parameters
	zetacoreClient interfaces.ZetacoreClient, // Zetacore client
	tss interfaces.TSSSigner, // TSS signer
	database *db.DB, // Database instance
	logger base.Logger, // Logger instance
	ts *metrics.TelemetryServer, // Telemetry server
) (*Observer, error) {
	// create base observer
	baseObserver, err := base.NewObserver(
		evmCfg.Chain,
		chainParams,
		zetacoreClient,
		tss,
		base.DefaultBlockCacheSize,
		base.DefaultHeaderCacheSize,
		ts,
		database,
		logger,
	)
	if err != nil {
		return nil, errors.Wrap(err, "unable to create base observer")
	}

	// create evm observer
	ob := &Observer{
		Observer:                      *baseObserver,
		evmClient:                     evmClient,
		evmJSONRPC:                    ethrpc.NewEthRPC(evmCfg.Endpoint),
		outboundPendingTransactions:   make(map[string]*ethtypes.Transaction),
		outboundConfirmedReceipts:     make(map[string]*ethtypes.Receipt),
		outboundConfirmedTransactions: make(map[string]*ethtypes.Transaction),
		priorityFeeConfig:             priorityFeeConfig{},
	}

	// load last block scanned
	if err = ob.LoadLastBlockScanned(ctx); err != nil {
		return nil, errors.Wrap(err, "unable to load last block scanned")
	}

	return ob, nil
}

---

Line range hint 79-81:
Ensure thread safety.

Consider adding a mutex lock to ensure thread safety when updating the evmClient field.

func (ob *Observer) WithEvmClient(client interfaces.EVMRPCClient) {
	ob.Mu().Lock()
	defer ob.Mu().Unlock()
	ob.evmClient = client
}

---

Line range hint 83-85:
Ensure thread safety.

Consider adding a mutex lock to ensure thread safety when updating the evmJSONRPC field.

func (ob *Observer) WithEvmJSONRPC(client interfaces.EVMJSONRPCClient) {
	ob.Mu().Lock()
	defer ob.Mu().Unlock()
	ob.evmJSONRPC = client
}

zetaclient/chains/bitcoin/observer/inbound.go (3)

Line range hint 1-13:
Add package-level documentation.

Consider adding documentation at the package level to describe the purpose and functionality of the observer package.

// Package observer implements the Bitcoin chain observer, responsible for monitoring and interacting with Bitcoin-based blockchains.
package observer

---

Line range hint 15-25:
Consider adding comments to struct fields.

Adding comments to the fields of the Observer struct can improve readability and maintainability.

// Observer is the observer for Bitcoin chains
type Observer struct {
	// base.Observer implements the base chain observer
	base.Observer

	// btcClient is the Bitcoin client for the observed chain
	btcClient interfaces.BTCRPCClient

	// netParams are the network parameters for the observed chain
	netParams *chaincfg.Params
}

---

Line range hint 27-50:
Remove unused variable.

The app variable is no longer used and can be removed from the WatchInbound function.

- app, err := zctx.FromContext(ctx)
- if err != nil {
-   return err
- }

zetaclient/zetacore/tx_test.go (2)

242-243: Add comments to the new variable.

Adding comments to the ethChainParams variable can improve readability and maintainability.

// ethChainParams simulates Ethereum chain parameters for testing.
ethChainParams := mocks.MockChainParams(chains.Ethereum.ChainId, 100)

---

276-277: Add comments to the new chain parameters.

Adding comments to the new chain parameters can improve readability and maintainability.

ChainParams: []*observertypes.ChainParams{
	{ChainId: 7000}, // ZetaChain
	&ethChainParams, // Ethereum chain parameters
},

zetaclient/orchestrator/orchestrator.go (1)

238-241: Update foreignChains to chainIDs in implementation files

The term foreignChains should be updated to chainIDs in the following implementation files to maintain consistency with the new approach in GetPendingCctxsWithinRateLimit:

• x/observer/keeper/chain_params.go
• x/crosschain/keeper/grpc_query_cctx_rate_limit.go

Please ensure these updates are made to reflect the new handling of chain IDs.

Analysis chain

LGTM! Verify consistency of chainIDs references.

The changes reflect the new approach to handle chains by using chainIDs. Ensure that all references to foreignChains have been updated to chainIDs throughout the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references to `foreignChains` have been updated to `chainIDs`.

# Test: Search for occurrences of `foreignChains`. Expect: No occurrences found.
rg --type go 'foreignChains'

Length of output: 933

---

Script:

#!/bin/bash
# Description: Display the surrounding lines of occurrences of `foreignChains` to understand their context.

# Display 5 lines before and after each occurrence of `foreignChains` to understand the context.
rg --type go 'foreignChains' -C 5

Length of output: 7402

changelog.md (1)

68-68: Ensure detailed information in changelog entry.

The changelog entry for PR #2568 is brief. Consider providing more details about the improvements to AppContext to help users understand the changes better.

- * [2568](https://github.com/zeta-chain/node/pull/2568) - improve AppContext
+ * [2568](https://github.com/zeta-chain/node/pull/2568) - improve AppContext by consolidating chain management components into a single structure, simplifying interactions and enhancing maintainability.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 545896d and 09478d6.

Files selected for processing (36)

• changelog.md (1 hunks)
• cmd/zetaclientd/debug.go (7 hunks)
• cmd/zetaclientd/start.go (4 hunks)
• pkg/slices/slices.go (1 hunks)
• zetaclient/chains/bitcoin/observer/inbound.go (4 hunks)
• zetaclient/chains/bitcoin/observer/observer.go (3 hunks)
• zetaclient/chains/bitcoin/observer/outbound.go (1 hunks)
• zetaclient/chains/evm/observer/inbound.go (8 hunks)
• zetaclient/chains/evm/observer/inbound_test.go (2 hunks)
• zetaclient/chains/evm/observer/observer.go (3 hunks)
• zetaclient/chains/evm/observer/observer_test.go (3 hunks)
• zetaclient/chains/evm/observer/outbound.go (1 hunks)
• zetaclient/chains/evm/observer/outbound_test.go (2 hunks)
• zetaclient/chains/evm/signer/outbound_data.go (4 hunks)
• zetaclient/chains/evm/signer/outbound_data_test.go (3 hunks)
• zetaclient/chains/evm/signer/signer.go (9 hunks)
• zetaclient/chains/evm/signer/signer_test.go (14 hunks)
• zetaclient/chains/interfaces/interfaces.go (1 hunks)
• zetaclient/chains/solana/observer/inbound.go (1 hunks)
• zetaclient/chains/solana/observer/inbound_tracker.go (1 hunks)
• zetaclient/config/types.go (1 hunks)
• zetaclient/context/app.go (3 hunks)
• zetaclient/context/app_test.go (1 hunks)
• zetaclient/context/chain.go (1 hunks)
• zetaclient/context/chain_test.go (1 hunks)
• zetaclient/orchestrator/bootstap_test.go (4 hunks)
• zetaclient/orchestrator/bootstrap.go (4 hunks)
• zetaclient/orchestrator/orchestrator.go (6 hunks)
• zetaclient/orchestrator/orchestrator_test.go (6 hunks)
• zetaclient/testutils/mocks/chain_params.go (1 hunks)
• zetaclient/testutils/mocks/zetacore_client.go (2 hunks)
• zetaclient/zetacore/client.go (2 hunks)
• zetaclient/zetacore/client_query_observer.go (2 hunks)
• zetaclient/zetacore/client_query_test.go (1 hunks)
• zetaclient/zetacore/client_worker.go (1 hunks)
• zetaclient/zetacore/tx_test.go (4 hunks)

Additional context used

Path-based instructions (35)

pkg/slices/slices.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/zetacore/client_worker.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

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.

zetaclient/chains/solana/observer/inbound_tracker.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/context/chain_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/outbound_data_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/context/chain.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/config/types.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

cmd/zetaclientd/debug.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/zetacore/client_query_observer.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/chains/evm/signer/outbound_data.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/chains/interfaces/interfaces.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.

zetaclient/chains/solana/observer/inbound.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.

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.

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/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/inbound.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/zetacore/tx_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/chains/evm/observer/outbound.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/orchestrator/orchestrator.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.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

zetaclient/testutils/mocks/zetacore_client.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/bitcoin/observer/outbound.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.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/zetacore/client_query_test.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 (2)

Learnt from: ws4charlie
PR: zeta-chain/node#2411
File: zetaclient/orchestrator/orchestrator.go:222-237
Timestamp: 2024-07-04T23:47:56.072Z
Learning: The `GetUpdatedChainObserver` method in the `Orchestrator` class is covered by unit tests in `zetaclient/orchestrator/orchestrator_test.go` and `zetaclient/orchestrator/chain_activate_test.go`.

Learnt from: ws4charlie
PR: zeta-chain/node#2411
File: zetaclient/orchestrator/chain_activate.go:116-181
Timestamp: 2024-07-05T00:02:36.493Z
Learning: The `CreateSignerObserverEVM` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.

Additional comments not posted (83)

pkg/slices/slices.go (1)

4-12: Function Map looks good.

The implementation is correct and follows Go best practices. The use of generics is appropriate for this utility function.

zetaclient/zetacore/client_worker.go (1)

35-35: Ensure the UpdateAppContext function handles the removal of the boolean parameter correctly.

The removal of the boolean parameter false from the UpdateAppContext call indicates a change in the function's signature or its default behavior. Verify that this change does not introduce any unintended side effects.

zetaclient/testutils/mocks/chain_params.go (2)

18-26: Initialization with fallback mechanism looks good.

The fallback mechanism ensures that ConnectorContractAddress and Erc20CustodyContractAddress have default values if the chainID is not found in the respective maps. This enhances the robustness of the function.

---

31-41: Ensure the new fields in ChainParams are correctly utilized.

The addition of new fields to the ChainParams struct expands its utility. Verify that these fields are correctly utilized in other parts of the codebase.

Verification successful

---

New fields in ChainParams are correctly utilized.

The search results indicate that the new fields added to the ChainParams struct are indeed utilized across various parts of the codebase. Here are the observations for each field:

• ZetaTokenContractAddress: Found in zetaclient/orchestrator/orchestrator_test.go, zetaclient/testutils/mocks/chain_params.go, x/observer/types/params.pb.go, x/observer/types/chain_params_test.go, x/observer/types/chain_params.go, x/observer/keeper/vote_inbound.go, x/crosschain/keeper/evm_hooks.go, zetaclient/chains/evm/observer/inbound.go, e2e/e2etests/test_migrate_chain_support.go, and testutil/sample/observer.go.

• ConnectorContractAddress: Found in x/observer/types/chain_params.go, x/observer/types/params.pb.go, x/observer/types/chain_params_test.go, zetaclient/testutils/mocks/chain_params.go, zetaclient/orchestrator/orchestrator_test.go, zetaclient/orchestrator/orchestrator.go, x/crosschain/types/tx_body_verification.go, zetaclient/orchestrator/bootstrap.go, cmd/zetaclientd/debug.go, zetaclient/chains/evm/observer/outbound_test.go, zetaclient/chains/evm/observer/observer.go, and testutil/sample/observer.go.

• Erc20CustodyContractAddress: Found in zetaclient/orchestrator/bootstrap.go, zetaclient/orchestrator/orchestrator_test.go, zetaclient/orchestrator/orchestrator.go, zetaclient/testutils/mocks/chain_params.go, x/observer/types/params.pb.go, x/observer/types/chain_params_test.go, x/observer/types/chain_params.go, zetaclient/chains/evm/observer/observer.go, x/crosschain/keeper/msg_server_whitelist_erc20.go, x/crosschain/types/tx_body_verification.go, testutil/sample/observer.go, e2e/e2etests/test_migrate_chain_support.go, and cmd/zetaclientd/debug.go.

• InboundTicker: Found in zetaclient/testutils/mocks/chain_params.go, zetaclient/orchestrator/orchestrator_test.go, x/observer/types/params.pb.go, x/observer/types/chain_params_test.go, x/observer/types/chain_params.go, zetaclient/chains/solana/observer/inbound_tracker.go, zetaclient/chains/solana/observer/inbound.go, zetaclient/chains/evm/observer/inbound.go, zetaclient/chains/bitcoin/observer/inbound.go, and testutil/sample/observer.go.

• OutboundTicker: Found in zetaclient/orchestrator/orchestrator_test.go, zetaclient/testutils/mocks/chain_params.go, x/observer/types/params.pb.go, x/observer/types/chain_params_test.go, x/observer/types/chain_params.go, zetaclient/chains/bitcoin/observer/outbound.go, zetaclient/chains/evm/observer/outbound.go, and testutil/sample/observer.go.

• WatchUtxoTicker: Found in x/observer/types/params.pb.go, x/observer/types/chain_params.go, x/observer/types/chain_params_test.go, zetaclient/orchestrator/orchestrator_test.go, zetaclient/testutils/mocks/chain_params.go, zetaclient/chains/bitcoin/observer/observer.go, and testutil/sample/observer.go.

• GasPriceTicker: Found in x/observer/types/chain_params_test.go, x/observer/types/params.pb.go, x/observer/types/chain_params.go, zetaclient/testutils/mocks/chain_params.go, zetaclient/orchestrator/orchestrator_test.go, zetaclient/chains/bitcoin/observer/observer.go, zetaclient/chains/evm/observer/observer_gas.go, and testutil/sample/observer.go.

• OutboundScheduleInterval: Found in x/observer/types/params.pb.go, x/observer/types/chain_params_test.go, x/observer/types/chain_params.go, zetaclient/testutils/mocks/chain_params.go, zetaclient/orchestrator/orchestrator_test.go, zetaclient/orchestrator/orchestrator.go, and testutil/sample/observer.go.

• OutboundScheduleLookahead: Found in x/observer/types/chain_params_test.go, x/observer/types/chain_params.go, x/observer/types/params.pb.go, zetaclient/testutils/mocks/chain_params.go, zetaclient/orchestrator/orchestrator.go, zetaclient/orchestrator/orchestrator_test.go, and testutil/sample/observer.go.

• BallotThreshold: Found in x/observer/types/ballot.go, x/observer/types/ballot.pb.go, x/observer/types/message_set_ballot_threshold_test.go, x/observer/types/chain_params.go, x/observer/types/ballot_test.go, x/observer/types/params.pb.go, x/observer/types/chain_params_test.go, x/observer/keeper/grpc_query_chain_params_test.go, x/observer/keeper/msg_server_vote_block_header_test.go, x/observer/keeper/voting.go, x/observer/keeper/vote_outbound_test.go, x/observer/keeper/voting_test.go, x/observer/keeper/msg_server_vote_tss.go, x/observer/keeper/msg_server_vote_blame_test.go, x/observer/keeper/vote_inbound_test.go, x/observer/keeper/ballot_test.go, x/crosschain/keeper/msg_server_migrate_tss_funds_test.go, zetaclient/zetacore/client_query_test.go, zetaclient/testutils/mocks/chain_params.go, zetaclient/orchestrator/orchestrator_test.go, and testutil/sample/observer.go.

• MinObserverDelegation: Found in zetaclient/zetacore/client_query_test.go, zetaclient/testutils/mocks/chain_params.go, x/observer/types/chain_params_test.go, x/observer/types/keys.go, x/observer/types/params.pb.go, x/observer/types/errors.go, x/observer/types/chain_params.go, x/observer/keeper/voting.go, zetaclient/orchestrator/orchestrator_test.go, x/observer/keeper/hooks.go, x/observer/keeper/voting_test.go, x/observer/keeper/grpc_query_chain_params_test.go, x/crosschain/keeper/msg_server_migrate_tss_funds_test.go, and testutil/sample/observer.go.

The new fields are correctly utilized in the codebase, ensuring their integration into the system.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the new fields in `ChainParams` are correctly utilized.

# Test: Search for the usage of `ChainParams`. Expect: The new fields are correctly utilized.
rg --type go 'ChainParams'

Length of output: 116338

---

Script:

#!/bin/bash
# Description: Verify the new fields in `ChainParams` are correctly utilized.

# Search for the usage of new fields in `ChainParams`
fields=(
  "ZetaTokenContractAddress"
  "ConnectorContractAddress"
  "Erc20CustodyContractAddress"
  "InboundTicker"
  "OutboundTicker"
  "WatchUtxoTicker"
  "GasPriceTicker"
  "OutboundScheduleInterval"
  "OutboundScheduleLookahead"
  "BallotThreshold"
  "MinObserverDelegation"
)

for field in "${fields[@]}"; do
  echo "Searching for usage of field: $field"
  rg --type go "$field"
done

Length of output: 45318

zetaclient/chains/solana/observer/inbound_tracker.go (1)

36-36: LGTM! But verify the correctness of IsInboundObservationEnabled.

The simplification of the function call to IsInboundObservationEnabled is approved.

However, ensure that the IsInboundObservationEnabled function correctly determines the state without parameters.

Verification successful

---

The change is correct and consistent.

The IsInboundObservationEnabled function does not require parameters, and its usage across the codebase aligns with the change made in inbound_tracker.go.

• zetaclient/context/app.go: The function is defined without parameters.
• zetaclient/chains/solana/observer/inbound.go: The function is used without parameters.
• zetaclient/chains/evm/observer/inbound.go: The function is used without parameters.
• zetaclient/chains/bitcoin/observer/inbound.go: The function is used without parameters.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of `IsInboundObservationEnabled` function.

# Test: Search for the function definition. Expect: The function should not require parameters.
rg --type go 'func IsInboundObservationEnabled' zetaclient/context

Length of output: 68

---

Script:

#!/bin/bash
# Description: Verify the correctness of `IsInboundObservationEnabled` function.

# Test 1: Search for the function definition in the entire repository. Expect: The function should not require parameters.
rg --type go 'func IsInboundObservationEnabled'

# Test 2: Search for the usage of `IsInboundObservationEnabled` to ensure it aligns with the changes made.
rg --type go 'IsInboundObservationEnabled()'

Length of output: 991

zetaclient/context/chain_test.go (2)

11-71: Comprehensive test coverage for ChainRegistry.

The test function TestChainRegistry provides comprehensive coverage for various aspects of the ChainRegistry, including adding chains, handling errors, and retrieving chains.

Consider adding tests for edge cases such as removing chains and updating chain parameters.

---

74-79: Initialization of ChainParams is correct.

The function makeParams correctly initializes and returns ChainParams based on the provided chain ID and support status.

zetaclient/chains/evm/signer/outbound_data_test.go (3)

Line range hint 21-33:
Comprehensive test coverage for SetChainAndSender.

The test function TestSigner_SetChainAndSender provides comprehensive coverage for various statuses of the SetChainAndSender method.

---

Line range hint 35-50:
Comprehensive test coverage for SetupGas.

The test function TestSigner_SetupGas provides comprehensive coverage for both successful and error scenarios of the SetupGas method.

---

Line range hint 77-129:
Comprehensive test coverage for NewOutboundData.

The test function TestSigner_NewOutboundData provides comprehensive coverage for various scenarios, including success, skip, unknown chain, and setup gas error.

zetaclient/context/chain.go (11)

39-45: Constructor function looks good.

The NewChainRegistry function correctly initializes the ChainRegistry struct.

---

58-65: Function looks good.

The All function correctly returns all chains in the registry sorted by chain ID.

---

85-91: Function looks good.

The SetAdditionalChains function correctly sets additional chains to the registry with proper thread safety.

---

93-101: Function looks good.

The Delete function correctly deletes one or more chains from the registry with proper thread safety.

---

103-107: Function looks good.

The Has function correctly checks if the chain is in the registry.

---

109-115: Function looks good.

The ChainIDs function correctly returns a list of chain IDs in the registry with proper thread safety.

---

143-145: Function looks good.

The ID function correctly returns the ID of the chain.

---

147-149: Function looks good.

The Params function correctly returns the parameters of the chain.

---

156-158: Function looks good.

The IsEVM function correctly checks if the chain is an EVM chain.

---

160-162: Function looks good.

The IsUTXO function correctly checks if the chain is a UTXO chain.

---

164-166: Function looks good.

The IsSolana function correctly checks if the chain is a Solana chain.

cmd/zetaclientd/debug.go (2)

37-37: Variable looks good.

The homeDir variable correctly centralizes the home directory path for the ZetaCore application, improving maintainability.

---

40-45: Function looks good.

The init function correctly refactors the command initialization process and adds command flags, improving maintainability.

zetaclient/zetacore/client_query_observer.go (1)

99-113: LGTM! But verify the function usage in the codebase.

The code changes are approved.

However, ensure that all function calls to GetKeyGen handle the new return type correctly.

zetaclient/chains/evm/signer/outbound_data.go (3)

96-98: LGTM! Improved error handling.

The changes to improve error handling and provide more contextual information are approved.

---

142-150: LGTM! Improved chain retrieval logic.

The changes to improve the chain retrieval logic and provide more descriptive error messages are approved.

---

163-165: LGTM! Improved gas setup logic.

The changes to improve the gas setup logic and provide more descriptive error messages are approved.

zetaclient/context/app.go (8)

38-44: LGTM! Initialization of chainRegistry.

The changes to initialize the new chainRegistry field are approved.

---

54-56: LGTM! New function to retrieve chain by ID.

The addition of the GetChain function to retrieve a chain by its ID is approved.

---

59-61: LGTM! New function to list chain IDs.

The addition of the ListChainIDs function to return the list of existing chain IDs in the registry is approved.

---

63-64: LGTM! New function to list all chains.

The addition of the ListChains function to return the list of all chains in the registry is approved.

---

68-82: LGTM! New function to return the first chain that satisfies the filter.

The addition of the FirstChain function to return the first chain that satisfies the filter is approved.

---

86-87: LGTM! Updated outbound observation check.

The changes to update the outbound observation check using the new crosschainFlags field are approved.

---

91-92: LGTM! Updated inbound observation check.

The changes to update the inbound observation check using the new crosschainFlags field are approved.

---

129-166: LGTM! Improved error handling and validation in Update function.

The changes to improve error handling and validation checks in the Update function are approved.

zetaclient/chains/interfaces/interfaces.go (1)

109-109: Verify the impact of changing the return type.

Changing the return type from a pointer to a value type can have significant implications. Ensure that all implementations and usages of this method are updated accordingly.

Verification successful

---

Verify the impact of changing the return type.

The change in the return type of the GetKeyGen method from a pointer to a value type has been correctly handled in the following instances:

• Test Case:

  • zetaclient/zetacore/client_query_test.go: The test case TestZetacore_GetKeyGen has been updated to compare the dereferenced expected output with the response.

• Implementation:

  • zetaclient/zetacore/client_query_observer.go: The implementation of GetKeyGen has been updated to return the value type.
  • zetaclient/testutils/mocks/zetacore_client.go: The mock implementation of GetKeyGen has been updated to return the value type.

• Usages:

  • zetaclient/zetacore/client.go: The usage of GetKeyGen has been updated to handle the value type correctly.
  • zetaclient/tss/tss_signer.go: The usage of GetKeyGen has been updated to handle the value type correctly.

All identified instances have been updated to handle the value type correctly.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all implementations and usages of the GetKeyGen method are updated to handle the new return type.

# Test: Search for the method implementation. Expect: All occurrences should handle the value type correctly.
rg --type go -A 5 $'GetKeyGen'

Length of output: 3883

---

Script:

#!/bin/bash
# Extract the implementation of GetKeyGen in client_query_observer.go
rg --type go -A 10 'func (c \*Client) GetKeyGen' zetaclient/zetacore/client_query_observer.go

# Extract the usage of GetKeyGen in client_query_test.go
rg --type go -A 10 'GetKeyGen' zetaclient/zetacore/client_query_test.go

# Extract the usage of GetKeyGen in client.go
rg --type go -A 10 'GetKeyGen' zetaclient/zetacore/client.go

# Extract the usage of GetKeyGen in tss_signer.go
rg --type go -A 10 'GetKeyGen' zetaclient/tss/tss_signer.go

# Extract the mock implementation of GetKeyGen in zetacore_client.go
rg --type go -A 10 'func \(_m \*ZetacoreClient\) GetKeyGen' zetaclient/testutils/mocks/zetacore_client.go

Length of output: 1972

zetaclient/chains/solana/observer/inbound.go (1)

55-55: Ensure correctness of the simplified condition.

The condition app.IsInboundObservationEnabled() has been simplified by removing the argument. Verify that this change does not affect the correctness of the inbound observation check.

Verification successful

---

The simplified condition is correct.

The method IsInboundObservationEnabled in AppContext does not require any parameters, confirming that the simplification of the condition in WatchInbound is correct.

• zetaclient/context/app.go: func (a *AppContext) IsInboundObservationEnabled() bool

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the simplified condition in `WatchInbound`.

# Test: Search for the definition of `IsInboundObservationEnabled` to ensure it does not require parameters.
rg --type go --context 5 'func IsInboundObservationEnabled'

Length of output: 61

---

Script:

#!/bin/bash
# Description: Verify the correctness of the simplified condition in `WatchInbound`.

# Test: Search for the definition of `IsInboundObservationEnabled` across the entire codebase.
rg --type go 'func IsInboundObservationEnabled'

Length of output: 49

---

Script:

#!/bin/bash
# Description: Verify the correctness of the simplified condition in `WatchInbound`.

# Test: Search for method definitions and interface implementations of `IsInboundObservationEnabled`.
rg --type go 'IsInboundObservationEnabled'

Length of output: 937

zetaclient/zetacore/client.go (3)

344-344: Ensure correctness of the simplified method signature.

The method signature has been simplified by removing the init and sampledLogger parameters. Verify that this change does not affect the method's functionality.

Verification successful

---

Ensure correctness of the simplified method signature.

The method signature has been simplified by removing the init and sampledLogger parameters. The search results indicate that all calls to UpdateAppContext match the new signature, suggesting the change was correctly propagated.

• zetaclient/zetacore/client_worker.go
• zetaclient/zetacore/tx_test.go
• cmd/zetaclientd/debug.go
• cmd/zetaclientd/start.go

Please verify that the removal of the init and sampledLogger parameters does not affect the method's functionality. Ensure that any initialization or logging previously handled by these parameters is now appropriately managed within the method or elsewhere in the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the simplified method signature in `UpdateAppContext`.

# Test: Search for all calls to `UpdateAppContext` to ensure they match the new signature.
rg --type go --context 5 'UpdateAppContext'

Length of output: 18456

---

369-369: Ensure proper handling of supported chains.

Verify that the method correctly handles supported chains and updates the appContext accordingly.

Verification successful

---

Ensure proper handling of supported chains.

The GetSupportedChains method appears to be correctly implemented to fetch supported chains and handle them appropriately. The method is used in various parts of the codebase, including updating the application context and iterating over supported chains to perform necessary operations.

• File: zetaclient/zetacore/client.go
  • Lines: 369-369
  • Snippet: supportedChains, err := c.GetSupportedChains(ctx)
  • Usage: The fetched supportedChains are used to update the application context and handle chain-specific logic.

Based on the gathered context, the handling of supported chains seems to be properly implemented.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the handling of supported chains in `UpdateAppContext`.

# Test: Search for the definition of `GetSupportedChains` to ensure it returns the expected chains.
rg --type go --context 5 'func GetSupportedChains'

Length of output: 52

---

Script:

#!/bin/bash
# Description: Verify the handling of supported chains in `UpdateAppContext`.

# Test 1: Search for the definition of `GetSupportedChains` using a broader pattern.
rg --type go --context 5 'GetSupportedChains'

# Test 2: Search for any function definitions in the codebase that might relate to supported chains.
rg --type go --context 5 'func.*SupportedChains'

# Test 3: Search for the usage of `GetSupportedChains` to understand its context.
rg --type go --context 5 'GetSupportedChains'

Length of output: 112728

---

Script:

#!/bin/bash
# Description: Verify the handling of supported chains in `UpdateAppContext`.

# Test: Search for the usage of `supportedChains` to understand how it updates `appContext`.
rg --type go --context 5 'supportedChains'

Length of output: 15987

---

379-379: Ensure proper handling of chain parameters.

Verify that the method correctly handles chain parameters and updates the appContext accordingly.

Verification successful

---

Ensure proper handling of chain parameters.

The UpdateAppContext method in zetaclient/zetacore/client.go correctly retrieves chain parameters using GetChainParams, validates them, and updates the appContext accordingly. The method includes comprehensive error handling and logging to ensure robustness.

• Location:
  • zetaclient/zetacore/client.go, lines 344-442

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the handling of chain parameters in `UpdateAppContext`.

# Test: Search for the definition of `GetChainParams` to ensure it returns the expected parameters.
rg --type go --context 5 'func GetChainParams'

Length of output: 48

---

Script:

#!/bin/bash
# Description: Verify the handling of chain parameters in `UpdateAppContext`.

# Test: Search for functions related to chain parameters in the codebase.
rg --type go --context 5 'func.*ChainParams'

Length of output: 131052

---

Script:

#!/bin/bash
# Description: Verify how chain parameters are used to update the appContext.

# Test: Search for the `UpdateAppContext` method in the codebase.
rg --type go --context 5 'func.*UpdateAppContext'

Length of output: 3249

---

Script:

#!/bin/bash
# Description: Extract the implementation of the `UpdateAppContext` method.

# Test: Retrieve the `UpdateAppContext` method implementation from `zetaclient/zetacore/client.go`.
ast-grep --lang go --pattern $'func (c *Client) UpdateAppContext(ctx context.Context, appContext *zctx.AppContext, logger zerolog.Logger) error {
  $$$
}' zetaclient/zetacore/client.go

Length of output: 6191

zetaclient/orchestrator/bootstap_test.go (3)

424-438: Ensure correctness of the adjusted function.

The mustUpdateAppContext function has been adjusted to accommodate the new signature of chainParams. Verify that the function correctly updates the application context.

Verification successful

---

Ensure correctness of the adjusted function.

The mustUpdateAppContext function has been adjusted to accommodate the new signature of chainParams. The function is correctly updating the application context in various test scenarios.

• The function mustUpdateAppContext is used multiple times in zetaclient/orchestrator/bootstap_test.go to update the application context with different sets of chains and chain parameters.
• Each call to mustUpdateAppContext is followed by actions that depend on the updated application context, confirming its correct implementation.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the adjusted `mustUpdateAppContext` function.

# Test: Search for all calls to `mustUpdateAppContext` to ensure they update the application context correctly.
rg --type go --context 5 'mustUpdateAppContext'

Length of output: 11331

---

391-411: Ensure correctness of the refactored function.

The chainParams function has been refactored to return supported chains and a single map of chain parameters. Verify that the function correctly handles different chain types.

Verification successful

---

The refactored chainParams function appears to be correct.

The function correctly handles different chain types (Bitcoin, Solana, EVM) and sets their parameters appropriately. The returned parameters are used correctly in various parts of the codebase, ensuring that the function's refactoring does not introduce any issues.

• The function is called and used in multiple files, including zetaclient/orchestrator/bootstap_test.go, zetaclient/zetacore/client.go, and others.
• The parameters returned by the function are utilized correctly, ensuring that the chain-specific logic is maintained.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the refactored `chainParams` function.

# Test: Search for all calls to `chainParams` to ensure they handle different chain types correctly.
rg --type go --context 5 'chainParams'

Length of output: 106662

---

419-420: Ensure proper update of the application context.

Verify that the mustUpdateAppContextChainParams function correctly updates the application context with the provided chain parameters.

zetaclient/orchestrator/orchestrator_test.go (4)

99-118: Updated test case to reflect new logic in CreateAppContext.

The modifications ensure that the test case accurately reflects the new logic introduced in CreateAppContext, improving the robustness of the tests.

---

Line range hint 129-191:
Updated test case to reflect new logic in CreateAppContext.

The modifications ensure that the test case accurately reflects the new logic introduced in CreateAppContext, improving the robustness of the tests.

---

362-365: Updated test case to reflect new logic in CreateAppContext.

The modifications ensure that the test case accurately reflects the new logic introduced in CreateAppContext, improving the robustness of the tests.

---

Line range hint 60-91:
Enhanced error handling in CreateAppContext.

The addition of the *testing.T parameter and improved error handling enhance the robustness of the tests. Ensure that all function calls to CreateAppContext match the new signature.

Verification successful

---

Enhanced error handling in CreateAppContext verified.

All instances of CreateAppContext in orchestrator_test.go match the new signature, including the *testing.T parameter, ensuring improved error handling and robustness of the tests.

• Verified instances in:
  • zetaclient/orchestrator/orchestrator_test.go

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `CreateAppContext` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'CreateAppContext'

Length of output: 4391

cmd/zetaclientd/start.go (3)

145-149: Streamlined error handling in UpdateAppContext.

The error handling for updating the application context has been streamlined, improving code readability and conciseness.

---

216-220: Improved error handling for Bitcoin chain ID retrieval.

The refactoring of how the Bitcoin chain ID is determined enhances error handling by ensuring that if the Bitcoin chain cannot be found, an error is returned with a descriptive message.

---

268-268: Updated check for enabled chains.

The check for enabled chains has been modified from appContext.GetEnabledChains() to appContext.ListChainIDs(), which may reflect a change in how enabled chains are managed or represented in the application context.

zetaclient/chains/evm/observer/observer_test.go (2)

111-111: Updated function to reflect new logic in getAppContext.

The modifications ensure that the function accurately reflects the new logic introduced in getAppContext, improving the robustness of the tests.

---

Line range hint 39-75:
Enhanced error handling and logging in getAppContext.

The addition of the *testing.T parameter, improved logging mechanism, and refined error handling enhance the testing framework's integration and robustness.

Verification successful

---

Enhanced error handling and logging in getAppContext.

The addition of the *testing.T parameter, improved logging mechanism, and refined error handling enhance the testing framework's integration and robustness.

The verification confirms that all calls to getAppContext match the new signature.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `getAppContext` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'getAppContext'

Length of output: 1525

---

Script:

#!/bin/bash
# Description: Verify all function calls to `getAppContext` match the new signature across the entire codebase.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 'getAppContext'

Length of output: 1525

zetaclient/chains/evm/observer/observer.go (7)

Line range hint 43-65:
Ensure proper error handling.

The error handling in the NewObserver function is appropriate. The function correctly wraps errors with context-specific messages.

---

Line range hint 87-92:
Ensure thread safety.

The SetChainParams function correctly uses a mutex lock to ensure thread safety when updating the chain parameters.

---

Line range hint 94-98:
Ensure thread safety.

The GetChainParams function correctly uses a mutex lock to ensure thread safety when retrieving the chain parameters.

---

Line range hint 100-105:
Ensure proper error handling.

The GetConnectorContract function handles errors appropriately when retrieving the connector contract.

---

Line range hint 107-112:
Ensure proper error handling.

The GetConnectorContractEth function handles errors appropriately when retrieving the Eth connector contract.

---

Line range hint 114-119:
Ensure proper error handling.

The GetERC20CustodyContract function handles errors appropriately when retrieving the ERC20Custody contract.

---

Line range hint 121-126:
Ensure proper error handling.

The FetchConnectorContractEth function handles errors appropriately when retrieving the Eth connector contract.

zetaclient/chains/bitcoin/observer/inbound.go (1)

Line range hint 198-220:
Ensure proper error handling.

The ProcessInboundTrackers function handles errors appropriately when processing inbound trackers.

zetaclient/zetacore/tx_test.go (1)

373-373: Improve logging practices.

The change to use zerolog.New(zerolog.NewTestWriter(t)) for logging during testing is appropriate and enhances the clarity and context of log outputs during test execution.

zetaclient/chains/evm/signer/signer_test.go (2)

165-165: Update context creation in tests.

The context creation in the test functions has been updated to use makeCtx(t) instead of makeCtx(). This ensures that any errors during context setup are properly handled and reported in the test output.

Also applies to: 188-188, 225-225, 265-265, 305-305, 344-344, 390-390, 431-431, 486-486, 533-533

---

572-586: Ensure proper error handling in context creation.

The makeCtx function now accepts a *testing.T parameter and uses require.NoError to handle errors during context setup. This improves the robustness of the tests by ensuring that any issues during context initialization are caught and reported correctly.

zetaclient/chains/evm/observer/outbound.go (1)

61-61: Simplify outbound observation check.

The GetChainParams() method call has been removed from the condition checking whether outbound observation is enabled. This change simplifies the logic by relying on a default or global setting for outbound observation. Ensure that this change does not affect the intended behavior of the WatchOutbound function.

Verification successful

---

Simplify outbound observation check.

The GetChainParams function is used extensively within the WatchOutbound function to fetch parameters such as OutboundTicker. Removing GetChainParams from the condition checking whether outbound observation is enabled does not appear to affect the intended behavior of the WatchOutbound function. The function still relies on GetChainParams for other critical operations.

• Locations:
  • zetaclient/chains/evm/observer/outbound.go: Usage of GetChainParams for OutboundTicker and other parameters.
  • zetaclient/chains/bitcoin/observer/outbound.go: Similar usage for OutboundTicker.

Category:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing GetChainParams() from the outbound observation check.

# Test: Search for the usage of GetChainParams in the codebase. Expect: No critical dependencies.
rg --type go $'GetChainParams'

Length of output: 33776

---

Script:

#!/bin/bash
# Search for the definition and usages of the WatchOutbound function to understand its dependencies
rg --type go 'func WatchOutbound' -A 10
rg --type go 'WatchOutbound' -A 10

Length of output: 15603

zetaclient/chains/evm/observer/inbound_test.go (1)

86-88: Refactor parameter handling in tests.

The chainID variable has been replaced with a direct invocation of mocks.MockChainParams using a hardcoded value of 1 for the Ethereum chain. This change enhances clarity by eliminating the previous assignment of chainID and directly using the mock parameters. Ensure that this change maintains the correctness and readability of the tests.

Also applies to: 95-96, 105-107

zetaclient/orchestrator/orchestrator.go (4)

155-157: LGTM! Ensure the reliability of chain.Params().

The changes simplify the logic for determining if a chain is an EVM chain and updating the zeta connector and ERC20 custody addresses. Ensure that the chain.Params() method is reliable and consistently returns the expected parameters.

---

204-207: LGTM! Ensure the reliability of chain.Params().

The changes simplify the logic for updating observer parameters by consolidating the retrieval of chain parameters. Ensure that the chain.Params() method is reliable and consistently returns the expected parameters.

---

383-385: LGTM! Ensure the reliability of the new chain parameters retrieval method.

The changes ensure that the latest chain parameters are used for scheduling EVM outbound keysigns. Ensure that the new chain parameters retrieval method is reliable and consistently returns the expected parameters.

---

337-340: LGTM! Verify consistency of the new approach to handle chains.

The changes simplify the logic for iterating over chains and querying pending cross-chain transactions. Ensure that the new approach to handle chains is consistent and that all necessary updates have been made.

Verification successful

---

The new approach to handle chains using ListChainIDs is consistent across the codebase.

The function ListChainIDs is used in multiple places, ensuring that the new approach is integrated and tested. The changes simplify the logic for iterating over chains and querying pending cross-chain transactions.

• cmd/zetaclientd/start.go
• zetaclient/orchestrator/orchestrator.go
• zetaclient/context/app_test.go
• zetaclient/context/app.go

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistency of the new approach to handle chains.

# Test: Search for occurrences of `GetEnabledExternalChains`. Expect: No occurrences found.
rg --type go 'GetEnabledExternalChains'

Length of output: 39

---

Script:

#!/bin/bash
# Description: Verify the consistency of the new approach to handle chains by searching for occurrences of `ListChainIDs`.

# Test: Search for occurrences of `ListChainIDs`. Expect: Occurrences found where the function is used to handle chains.
rg --type go 'ListChainIDs'

Length of output: 488

zetaclient/chains/bitcoin/observer/observer.go (1)

Line range hint 1-7:
LGTM! Verify the impact of the removal of postBlockHeader.

The removal of the postBlockHeader function and the proofs package import suggests a refactoring effort to simplify the codebase. Ensure that this removal does not affect other parts of the codebase and that the functionality is either deprecated or replaced.

zetaclient/testutils/mocks/zetacore_client.go (1)

286-301: LGTM! Verify the consistency of handling the new return type.

The changes streamline the return value handling for the GetKeyGen function, eliminating unnecessary pointer dereferencing. Ensure that the new return type is handled consistently throughout the codebase.

Verification successful

---

The return type of GetKeyGen is handled consistently and correctly across the codebase.

The function's return type is appropriately managed as a value in all relevant files, ensuring no issues with pointer dereferencing.

• zetaclient/zetacore/client_query_test.go
• zetaclient/zetacore/client_query_observer.go
• zetaclient/zetacore/client.go
• zetaclient/tss/tss_signer.go
• zetaclient/chains/interfaces/interfaces.go

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistency of handling the new return type for `GetKeyGen`.

# Test: Search for occurrences of `GetKeyGen`. Expect: No occurrences of pointer type found.
rg --type go 'GetKeyGen'

Length of output: 1043

---

Script:

#!/bin/bash
# Description: Verify the consistency of handling the new return type for `GetKeyGen` in the identified files.

# Check return type handling in client_query_test.go
rg --type go 'GetKeyGen' zetaclient/zetacore/client_query_test.go -A 5

# Check return type handling in client_query_observer.go
rg --type go 'GetKeyGen' zetaclient/zetacore/client_query_observer.go -A 5

# Check return type handling in client.go
rg --type go 'GetKeyGen' zetaclient/zetacore/client.go -A 5

# Check return type handling in tss_signer.go
rg --type go 'GetKeyGen' zetaclient/tss/tss_signer.go -A 5

# Check return type handling in zetacore_client.go (already reviewed, skipping)

# Check return type handling in interfaces.go
rg --type go 'GetKeyGen' zetaclient/chains/interfaces/interfaces.go -A 5

Length of output: 1607

zetaclient/chains/evm/observer/outbound_test.go (1)

201-201: LGTM! Verify the correctness of the mock setup.

The replacement of observertypes.ChainParams with mocks.MockChainParams should enhance test flexibility and accuracy. Ensure that the mock setup aligns with the test's requirements.

zetaclient/chains/bitcoin/observer/outbound.go (1)

55-57: LGTM! Verify the method's functionality.

The removal of parameters from the IsOutboundObservationEnabled call simplifies the method. Ensure that the method's functionality remains intact and that the change does not introduce any issues.

zetaclient/chains/evm/observer/inbound.go (5)

60-62: LGTM! Verify the method's functionality.

The removal of parameters from the IsInboundObservationEnabled call simplifies the method. Ensure that the method's functionality remains intact and that the change does not introduce any issues.

---

100-102: LGTM! Verify the method's functionality.

The removal of parameters from the IsInboundObservationEnabled call simplifies the method. Ensure that the method's functionality remains intact and that the change does not introduce any issues.

---

417-417: LGTM! Verify the method's functionality.

The direct assignment of the chainID variable from the observer's chain ID simplifies the method's flow. Ensure that the method's functionality remains intact and that the change does not introduce any issues.

---

650-655: LGTM! Verify the correctness of changes.

The simplifications in chain ID retrieval and corrections in variable names enhance clarity and maintainability. Ensure that these changes are correct and do not introduce any issues.

---

666-672: LGTM! Verify the correctness of changes.

The additional checks for potential attack attempts enhance security. Ensure that these changes are correct and do not introduce any issues.

zetaclient/chains/evm/signer/signer.go (4)

396-398: Enhanced error handling for chain retrieval.

The change from chains.GetChainFromChainID to app.GetChain improves error handling by returning an error if the chain cannot be retrieved. This ensures that the error is logged and handled appropriately.

---

450-452: Consistent logging format for chain ID.

The logging statements now use toChain.ID() instead of toChain.String(), which outputs the chain ID as an integer rather than a string. This change ensures a consistent and precise logging format.

Also applies to: 459-461, 468-470, 484-486, 494-496, 502-504, 516-518, 532-534, 546-548

---

572-574: Enhanced error handling for chain retrieval.

The change from chains.GetChainFromChainID to app.GetChain improves error handling by returning an error if the chain cannot be retrieved. This ensures that the error is logged and handled appropriately.

---

595-599: Consistent logging format for chain ID.

The logging statements now use toChain.ID() instead of toChain.String(), which outputs the chain ID as an integer rather than a string. This change ensures a consistent and precise logging format.

Also applies to: 603-603, 612-613

zetaclient/zetacore/client_query_test.go (1)

629-629: Improved precision in test assertions.

The change from require.Equal(t, expectedOutput.Keygen, resp) to require.Equal(t, *expectedOutput.Keygen, resp) ensures that the value being compared is the actual value pointed to by Keygen, rather than the pointer itself. This enhances the precision of the test.

[lumtis]

The PR mentions

Partially closes: #2519

But the issue is actually attached to the PR and would be closed by it

[lumtis]

It looks like a good improvement to me.

As discussed let's merge Solana PR first #2560 to simplify and reduce backport to release/v19

[swift1337]

setup      | ⏳ e2e tests passed,checking tx priority
setup      | ✅ e2e tests completed in 7m45.361153117s
setup      |  ---📈 Network Report ---
setup      | Block Height:           248
setup      | CCTX Processed:         51
setup      | Emissions Pool Balance: 19998075ZETA
e2e passed

[lumtis]

Just one comment then good to go to me

[skosito]

looks good, couple minor comments