Make new cobra CLI canon #1100
Make new cobra CLI canon #1100
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| OWNER_ADDR="$1" |
There was a problem hiding this comment.
The script will exit with an obscure “unbound variable” error if fewer than four arguments are provided, making it hard to understand what went wrong. This occurs because $1 through $4 are referenced under set -u without verifying that those positional parameters are set. Consider adding a check for the number of arguments at the top of the script and printing a clear usage message before accessing $1..$4 to improve usability.
+if [ "$#" -lt 4 ]; then
+ echo "Usage: $0 OWNER_ADDR SIGNER_PUB HTTP_ADDR CFG"
+ exit 1
+fi🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| signer, err := blockchain.NewPrivateKeySigner(privateKey, contracts.AppChain.ChainID) |
There was a problem hiding this comment.
Using the app-chain RPC client and signer for the ParameterAdmin will cause calls to the settlement-chain parameter registry to fail at runtime since it uses the wrong endpoint and signs with the app-chain chain ID. This happens because setupAppChainAdmin passes the same client and signer (tied to the app chain) into NewParameterAdmin. Consider creating a separate RPC client and signer configured with the settlement-chain RPC URL (or an additional flag) and contracts.SettlementChain.ChainID for constructing the ParameterAdmin so its calls go to the correct chain.
+ settlementSigner, err := blockchain.NewPrivateKeySigner(privateKey, contracts.SettlementChain.ChainID)
+ if err != nil {
+ return nil, fmt.Errorf("could not create settlement-chain signer: %w", err)
+ }
- paramAdmin, err := blockchain.NewParameterAdmin(logger, client, signer, contracts)
+ paramAdmin, err := blockchain.NewParameterAdmin(logger, client, settlementSigner, contracts)
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| } | ||
|
|
||
| func (b *PayloadBound) Set(s string) error { | ||
| switch strings.TrimSpace(s) { |
There was a problem hiding this comment.
Accepted inputs with leading or trailing whitespace get stored with that whitespace, which can lead to mismatches later on.
The code trims the input for validation in the switch but then assigns the original untrimmed s to *b.
Consider capturing the trimmed value in a variable and using that for assignment so the stored value is the canonical, validated string.
+ trimmed := strings.TrimSpace(s)
- switch strings.TrimSpace(s) {
+ switch trimmed {
- *b = PayloadBound(s)
+ *b = PayloadBound(trimmed)🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| return &cobra.Command{ | ||
| Use: "get", | ||
| Short: "Get PRM protocol fee rate (bps)", | ||
| RunE: func(*cobra.Command, []string) error { return settlePRMFeeRateGetHandler() }, |
There was a problem hiding this comment.
Users won't be able to cancel the get command with Ctrl-C or any parent context cancellation, and any flags or args parsed by Cobra aren’t available to the handler.
This happens because RunE calls a zero-argument handler, which always uses context.Background() instead of the command’s actual context and has no access to cmd or args.
Consider passing cmd (and args if needed) into the handler so it can use cmd.Context() for cancellation and inspect flags. For example, change RunE to func(cmd *cobra.Command, args []string) error { return settlePRMFeeRateGetHandler(cmd.Context()) } and update the handler signature accordingly.
- RunE: func(*cobra.Command, []string) error { return settlePRMFeeRateGetHandler() },
+ RunE: func(cmd *cobra.Command, args []string) error { return settlePRMFeeRateGetHandler(cmd.Context()) },
-func settlePRMFeeRateGetHandler() error {
- logger, err := cliLogger()
+func settlePRMFeeRateGetHandler(ctx context.Context) error {
+ logger, err := cliLogger()
- ctx := context.Background()
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| return nil, fmt.Errorf("could not load config from file: %w", err) | ||
| } | ||
|
|
||
| client, err := blockchain.NewRPCClient(ctx, rpcURL) |
There was a problem hiding this comment.
The RPC client created by blockchain.NewRPCClient via ethclient.DialContext in [file:cmd/xmtpd-cli/commands/admin_setup.go:79] and [file:cmd/xmtpd-cli/commands/admin_setup.go:123] isn't closed on error, leading to leaked network connections. After dialing, if any of NewPrivateKeySigner, NewParameterAdmin, or NewSettlementChainAdmin fails, the opened *ethclient.Client isn’t cleaned up. Consider deferring a conditional chainClient.Close() immediately after creation or explicitly closing the client on each early return so that connections are only kept open when admin creation succeeds.
- client, err := blockchain.NewRPCClient(ctx, rpcURL)
- if err != nil {
- return nil, err
- }
+ client, err := blockchain.NewRPCClient(ctx, rpcURL)
+ if err != nil {
+ return nil, err
+ }
+ // Close client on any setup failure to avoid resource leaks
+ defer func() {
+ if setupErr != nil {
+ client.Close()
+ }
+ }()
@@
- signer, err := blockchain.NewPrivateKeySigner(privateKey, contracts.SettlementChain.ChainID)
- if err != nil {
- return nil, fmt.Errorf("could not create signer: %w", err)
- }
+ signer, err := blockchain.NewPrivateKeySigner(privateKey, contracts.SettlementChain.ChainID)
+ if err != nil {
+ setupErr = fmt.Errorf("could not create signer: %w", err)
+ return nil, setupErr
+ }
@@
- paramAdmin, err := blockchain.NewParameterAdmin(logger, client, signer, contracts)
- if err != nil {
- return nil, fmt.Errorf("could not create parameter admin: %w", err)
- }
+ paramAdmin, err := blockchain.NewParameterAdmin(logger, client, signer, contracts)
+ if err != nil {
+ setupErr = fmt.Errorf("could not create parameter admin: %w", err)
+ return nil, setupErr
+ }
@@
- return blockchain.NewSettlementChainAdmin(logger, client, signer, contracts, paramAdmin)
+ admin, err := blockchain.NewSettlementChainAdmin(logger, client, signer, contracts, paramAdmin)
+ if err != nil {
+ setupErr = err
+ return nil, err
+ }
+ return admin, nil🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| fi | ||
|
|
||
| echo "==> Node id: ${NODE_ID}" | ||
|
|
There was a problem hiding this comment.
The enable step will treat any failure as “already enabled”, masking real errors (like network or auth failures) and still printing a green success checkmark.
This happens because the || echo "ℹ️ Node ${NODE_ID} already enabled" swallows all errors from the nodes canonical-network --add command.
Consider explicitly checking for the “already enabled” error string and exiting on any other failure. Also add a guard to ensure NODE_ID is non-empty before attempting the enable step.
+[[ -n "${NODE_ID}" ]] || { echo "ERROR: NODE_ID is empty"; exit 1; }
+if ! output=$(dev/cmd/cli \
+ --private-key "${PRIVATE_KEY}" \
+ --rpc-url "${RPC_URL}" \
+ --log-encoding json \
+ --config-file "${CFG}" \
+ nodes canonical-network --add --node-id "${NODE_ID}" 2>&1); then
+ if grep -q "already enabled" <<< "$output"; then
+ echo "ℹ️ Node ${NODE_ID} already enabled"
+ else
+ echo "ERROR: Enable failed: $output"; exit 1
+ fi
+fi🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
Adopt the new Cobra CLI as canonical by switching the build/entrypoint to
cmd/xmtpd-cliand adding top-levelappchainandsettlementadministrative command groupsThis change restructures the CLI around
cmd/xmtpd-cli, adds administrative command trees, consolidates admin initialization, and updates scripts and containers to the new command structure.RunEwith error propagation and updated flags/handlers in node_registry.go, key_management.go, and rate_registry.go.configureRootCmd, addcompletion, and change the default of--config-fileto empty in root.go.📍Where to Start
Start with
configureRootCmdin root.go, then review the command definitions in appchain.go and settlement.go, and the admin helpers in admin_setup.go.Macroscope summarized 6f40f23.