Skip to content

fix: correct action dry-run metadata#10

Merged
yongjun925 merged 1 commit into
mainfrom
fix/action-execution-metadata-1.2.1
Jul 13, 2026
Merged

fix: correct action dry-run metadata#10
yongjun925 merged 1 commit into
mainfrom
fix/action-execution-metadata-1.2.1

Conversation

@yongjun925

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

Copy link
Copy Markdown

Findings

  • [P1] Do not infer USD value from an untrusted token symbolsrc/commands/swap.rs:966
    Any ERC-20 can report USDC, USDT, etc. as its symbol. The new logic consequently marks arbitrary tokens as dollar-valued in risk.token_in.amount_usd, which downstream external signers may use for budget authorization. Validate (chain_id, address) against a trusted stablecoin registry, or omit amount_usd unless a reliable price source is available.

  • [P2] Omit risk.spender for native-token swapssrc/commands/swap.rs:1105
    The field is now populated with DODOApprove for every swap, including native ETH inputs, although native assets have no allowance or spender. This creates misleading authorization metadata. Populate it only when from_token is ERC-20; the existing test at line 2526 currently codifies the incorrect native-token case.

Project Checks

  • Output convention: complete. No new direct println!/print!; command output still uses print_output.
  • Help text: complete. No clap commands or flags changed, and existing dry-run help remains accurate.
  • Skills: missing update. skills/chainpilot/SKILL.md only describes risk generically and does not document the new spender, USD valuation, or minimum-output semantics.
  • READMEs: English and Chinese remain mutually synchronized, but both omit those user-facing dry-run metadata changes. Update both README.md and README_CN.md.
  • Tests could not run because dependencies were unavailable locally and network access to crates.io failed.

@yongjun925
yongjun925 force-pushed the fix/action-execution-metadata-1.2.1 branch from 8472afd to 3c34100 Compare July 13, 2026 09:05
@github-actions

Copy link
Copy Markdown

Findings

  • P1 — Update the skill’s amount-unit reference. skills/chainpilot/SKILL.md:169 still says to_amount_min is raw base units, and line 179 says the same for simulation min_out. This PR changes DODO quotes to store a display amount at src/api/dodo.rs:418, which simulation copies directly. Following the current skill would therefore divide these values twice and show an incorrect minimum output. Mark both fields as display-ready and remove the conversion instruction.

Project-specific checks:

  • Output convention: complete; no new direct stdout printing was introduced.
  • Help text: complete; no commands, arguments, or flags changed.
  • Skills: updated, but internally inconsistent as described above.
  • READMEs: README.md and README_CN.md contain equivalent updates and remain synchronized.

git diff --check passed. I could not run Rust tests because the workspace is read-only and Cargo requires build-output writes.

@yongjun925
yongjun925 force-pushed the fix/action-execution-metadata-1.2.1 branch from 3c34100 to 5db7dca Compare July 13, 2026 09:10
@github-actions

Copy link
Copy Markdown

No blocking findings in the PR changes.

Project-specific checks:

  • Output convention: Complete. No new direct println!/print!; command results continue through print_output.
  • Help docs: Complete. No CLI commands, flags, or arguments were added or changed.
  • Skills: Complete. skills/chainpilot/SKILL.md documents spender, USD metadata, and minimum-output semantics.
  • Multi-language README: Complete. README.md and README_CN.md contain equivalent updates.

The added tests cover ERC-20/native spender behavior, trusted stablecoin valuation, and legacy raw minimum-output compatibility. Tests were not executed because the review environment is read-only.

@yongjun925
yongjun925 merged commit d779bf1 into main Jul 13, 2026
5 checks passed
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