Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(e2e): use address_book.seed rather than bootstrap peer #3277

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

gartnera
Copy link
Member

@gartnera gartnera commented Dec 11, 2024

Generate the address_book.seed file for zetaclient when running in localnet. This is to ensure that the localnet/e2e setup matches the testnet/mainnet zetaclient configuration where we configure an address book.

This command lives under zetae2e rather than zetaclient as this is localnet/e2e specific functionality.

Related to zeta-chain/go-tss#40 (Remove all dynamic peer discovery)

Summary by CodeRabbit

  • New Features

    • Introduced a new CLI command for retrieving bootstrap address book entries for the Zeta client.
    • Enhanced the zetae2e command-line interface with the new command registration.
    • Modified the start-zetaclientd.sh script to streamline the initialization process and retrieve the bootstrap address book.
  • Bug Fixes

    • Improved error handling for gRPC client creation and hostname resolution in the new command.

@gartnera gartnera added no-changelog Skip changelog CI check PERFORMANCE_TESTS Run make start-e2e-performance-test labels Dec 11, 2024
@gartnera gartnera requested a review from a team as a code owner December 11, 2024 05:03
@gartnera gartnera changed the title refactor(e2e): use seed address book rather than bootstrap peer refactor(e2e): use address_book.seed rather than bootstrap peer Dec 11, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (2)
cmd/zetae2e/get_zetaclient_bootstrap.go (2)

68-68: Adhere to Go Naming Conventions for Acronyms

The variable peerId should be renamed to peerID to follow Go naming conventions for acronyms, enhancing code consistency and readability.

Apply this diff to correct the variable name:

-		peerId, err := conversion.Bech32PubkeyToPeerID(account.GranteePubkey.Secp256k1.String())
+		peerID, err := conversion.Bech32PubkeyToPeerID(account.GranteePubkey.Secp256k1.String())

Also, update subsequent references to peerID accordingly:

-		fmt.Printf("/ip4/%s/tcp/6668/p2p/%s\n", ipv4Address, peerId.String())
+		fmt.Printf("/ip4/%s/tcp/6668/p2p/%s\n", ipv4Address, peerID.String())
🧰 Tools
🪛 GitHub Check: lint

[failure] 68-68:
var-naming: var peerId should be peerID (revive)


55-57: Enhance Error Context by Wrapping Returned Errors

Returning errors with additional context aids in debugging and log tracing. Consider wrapping the error to include more detail.

Apply this diff to improve error handling:

			accAddr, err := sdk.AccAddressFromBech32(account.Operator)
			if err != nil {
-				return err
+				return fmt.Errorf("failed to convert operator address from Bech32: %w", err)
			}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 376b714 and 96786f0.

📒 Files selected for processing (3)
  • cmd/zetae2e/get_zetaclient_bootstrap.go (1 hunks)
  • cmd/zetae2e/root.go (1 hunks)
  • contrib/localnet/scripts/start-zetaclientd.sh (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
cmd/zetae2e/root.go (1)

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

contrib/localnet/scripts/start-zetaclientd.sh (1)

Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.

cmd/zetae2e/get_zetaclient_bootstrap.go (1)

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

🪛 GitHub Check: lint
cmd/zetae2e/get_zetaclient_bootstrap.go

[failure] 68-68:
var-naming: var peerId should be peerID (revive)

🔇 Additional comments (1)
cmd/zetae2e/root.go (1)

32-32: Command Registration Added Successfully

The new command NewGetZetaclientBootstrap() is properly registered, enhancing the CLI functionality.

cmd/zetae2e/get_zetaclient_bootstrap.go Outdated Show resolved Hide resolved
cmd/zetae2e/get_zetaclient_bootstrap.go Outdated Show resolved Hide resolved
contrib/localnet/scripts/start-zetaclientd.sh Outdated Show resolved Hide resolved
contrib/localnet/scripts/start-zetaclientd.sh Show resolved Hide resolved
cmd/zetae2e/get_zetaclient_bootstrap.go Outdated Show resolved Hide resolved
cmd/zetae2e/get_zetaclient_bootstrap.go Outdated Show resolved Hide resolved
@gartnera gartnera force-pushed the localnet-zetaclient-addressbook branch from ad4d4ef to f1ca865 Compare December 11, 2024 17:10
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.79%. Comparing base (6bbe9ea) to head (7b43854).
Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3277   +/-   ##
========================================
  Coverage    61.79%   61.79%           
========================================
  Files          429      429           
  Lines        30816    30816           
========================================
  Hits         19043    19043           
  Misses       10914    10914           
  Partials       859      859           

cmd/zetae2e/local/get_zetaclient_bootstrap.go Outdated Show resolved Hide resolved
@gartnera gartnera force-pushed the localnet-zetaclient-addressbook branch from 4a83f5d to 7b43854 Compare December 12, 2024 16:43
@gartnera gartnera enabled auto-merge December 12, 2024 16:44
Copy link
Contributor

@kingpinXD kingpinXD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NodeAccounts list can contain accounts which are not part of the TSS yet , is it okay to use the list for the seed ?

Okay dont think its a problem since we are using for starting the network only

@gartnera
Copy link
Member Author

NodeAccounts list can contain accounts which are not part of the TSS yet , is it okay to use the list for the seed ?

Okay dont hink its a problem since we are using for starting the network only

Yes I think the address book should contain all possible peers. Only those that are needed will be actually used.

@gartnera gartnera added this pull request to the merge queue Dec 12, 2024
@zeta-chain zeta-chain deleted a comment from coderabbitai bot Dec 12, 2024
Merged via the queue into develop with commit f63cccf Dec 12, 2024
44 checks passed
@gartnera gartnera deleted the localnet-zetaclient-addressbook branch December 12, 2024 17:13
skosito pushed a commit that referenced this pull request Dec 16, 2024
* refactor(e2e): use seed address book rather than bootstrap peer

* review updates

* move to local

* feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:cli no-changelog Skip changelog CI check PERFORMANCE_TESTS Run make start-e2e-performance-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants