Skip to content

Fix correctness bugs and clean up dead code across CLI#638

Open
Uxio0 wants to merge 1 commit into
mainfrom
fix/code-review-correctness-cleanups
Open

Fix correctness bugs and clean up dead code across CLI#638
Uxio0 wants to merge 1 commit into
mainfrom
fix/code-review-correctness-cleanups

Conversation

@Uxio0

@Uxio0 Uxio0 commented Jun 26, 2026

Copy link
Copy Markdown
Member

What

Fixes a batch of correctness bugs and removes dead code found during a full-codebase review. No new features, no behavior changes beyond fixing the bugs below.

Correctness fixes

tx-builder file decoder (tx_builder_file_decoder.py)

  • Preserve the array suffix for tuple-array types (tuple[], tuple[N]). Before, the [] was dropped, so both the 4-byte selector and the ABI encoding targeted the wrong (non-array) signature — a batch file calling a method with an array-of-structs argument silently built the wrong calldata.
  • parse_int_value now requires the 0x prefix for hex. Before, a bare string like "abc" / "deadbeef" was silently parsed base-16, producing a wrong amount.
  • Missing/null contractInputsValues no longer crashes with an opaque error; a missing per-field value now raises a clear Missing value for input 'X'.
  • value is coerced to int (missing/empty defaults to 0) instead of passing a string/None downstream.

tx-service operator (safe_tx_service_operator.py)

  • confirm_message and sign_message now return False after the "owner not loaded" / "no owner loaded" guards, instead of falling through to a None-deref crash or posting an empty signature.

safe operator (safe_operator.py)

  • No-arg SafeOperatorException subclasses no longer crash the generic handler (e.args[0] IndexError); GuardNotSupportedException / FallbackHandlerNotSupportedException now carry messages.
  • change_* / update_version* always return a bool (added the missing failure/success returns).
  • change_threshold rejects threshold < 1 locally.
  • Cached nonce is set to safe_tx.safe_nonce + 1 instead of a blind += 1, so the displayed nonce is correct when a custom --safe-nonce is used.
  • load_cli_owners_from_words reports an explicit empty-seed error for an unset/empty env var.

main / prompt parser

  • SAFE_CLI_INTERACTIVE="" is respected instead of silently ignored.
  • Default-command routing uses Web3.is_address, so a lowercase (non-checksummed) Safe address starts attended mode instead of erroring with "No such command".
  • process_command uses shlex.split, so quoted/multi-word args (e.g. delegate labels with spaces) parse correctly.

hardware wallet / hd wallet

  • Ledger exception wrapper matches the message with in str(e) instead of in e.args, so the USB-write recovery hint actually fires.
  • get_account_from_words no longer overrides a caller-supplied hd_path when index != 0.
  • Trezor legacy-tx data access made consistent with the other branches.

Cleanup

  • safe_lexer: fixed the address/tx-hash regex ({62}{64}, malformed [aA-zZ,0-9][a-fA-F0-9]).
  • safe_completer / safe_completer_constants: argument coloring now works for multi-arg commands; removed the dead safe_color_arguments dict, the dead change_owner meta entry, and fixed the change_threshold hint (<address><integer>).

Testing

  • ruff check and ruff format clean.
  • Full test suite: 83 passed, 1 skipped (ran against local ganache).

Notes / deliberately out of scope

  • Did not change LedgerWallet.get_address to pass self.dongle — it alters runtime behavior and breaks an existing test that encodes the old contract; real-world impact is low.
  • Did not change the --interactive vs SAFE_CLI_INTERACTIVE=0 precedence — current behavior matches the documented "only --non-interactive overrides" design.
  • Left the @cache on get_trezor_client (documented as intentional) and the argparse/typer validator duplication (larger refactor) for follow-ups.

@Uxio0 Uxio0 requested a review from a team as a code owner June 26, 2026 14:26

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9348e2b0ff

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/safe_cli/prompt_parser.py Outdated

def process_command(self, command: str):
args = self.prompt_parser.parse_args(command.split())
args = self.prompt_parser.parse_args(shlex.split(command))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Handle unmatched quotes before parsing commands

When an attended-mode user mistypes a quoted argument, for example add_delegate 0x... "label, shlex.split raises ValueError: No closing quotation; SafeCli.loop only catches argparse errors, SystemExit, EOF, and keyboard interrupts, so this new exception escapes and terminates the interactive session instead of showing a parse error and continuing.

Useful? React with 👍 / 👎.

Found during a full-codebase review. Main fixes:

- tx-builder decoder: preserve tuple-array suffix (tuple[], tuple[N]) so the
  selector and ABI encoding match; require 0x prefix for hex ints; clear errors
  for missing/null contract input values; coerce tx value to int
- tx-service operator: return after "owner not loaded" guards to avoid a
  None-deref crash and posting empty signatures
- safe_operator: harden no-arg exception handling, make change_*/update_version*
  always return bool, reject threshold < 1, fix cached nonce with custom
  --safe-nonce, clearer empty-seed error
- main: respect empty SAFE_CLI_INTERACTIVE, route lowercase addresses to
  attended mode
- prompt_parser: use shlex.split so quoted args parse correctly
- ledger: fix exception-message match for USB write errors
- hd wallet: stop overriding a custom hd_path when index is set
- lexer/completer: fix address/tx-hash regex, fix argument coloring, drop dead
  entries
@Uxio0 Uxio0 force-pushed the fix/code-review-correctness-cleanups branch from 9348e2b to 4860a7d Compare June 26, 2026 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant