Skip to content

fix(etherfi-plugin): fix approve race condition, JSON positions output, missing weETHExpected (v0.2.7)#212

Merged
purong-huang-1121 merged 1 commit into
okx:mainfrom
GeoGu360:fix/etherfi-plugin-approve-race-json-v0.2.7
Apr 15, 2026
Merged

fix(etherfi-plugin): fix approve race condition, JSON positions output, missing weETHExpected (v0.2.7)#212
purong-huang-1121 merged 1 commit into
okx:mainfrom
GeoGu360:fix/etherfi-plugin-approve-race-json-v0.2.7

Conversation

@GeoGu360
Copy link
Copy Markdown

Summary

  • EVM-006 (approve race condition): wrap and unstake commands used sleep(3s/15s) after the approve tx — not sufficient for Ethereum's ~12s average block time. Replaced with wait_for_tx() polling loop that checks onchainos wallet history --tx-hash every 3s (up to 90s timeout) and only proceeds once txStatus: SUCCESS.
  • EVM-002 (agent-unreadable output): positions printed a human-readable text table. Replaced with structured serde_json::json! output: eeth_balance, weeth_balance, weeth_as_eeth, total_eeth, total_usd, rate, apy_pct, tvl_usd, eth_price_usd.
  • EVM-012 (silent zero on RPC failure): positions used unwrap_or(0) for balance and exchange rate — returned 0 silently when RPC was down. Changed to fail-fast with explicit error messages.
  • wrap: Added missing weETHExpected field in output — computed via getRate() (weETH = eETH / rate).
  • unwrap: eETHExpected showed 18 decimal places. Changed to 6dp via format!("{:.6}", ...).

Files changed

  • src/onchainos.rs — added wait_for_tx() async + wait_for_tx_sync() polling impl
  • src/commands/wrap.rs — replaced sleep with wait_for_tx, added weETHExpected
  • src/commands/unstake.rs — replaced sleep with wait_for_tx
  • src/commands/positions.rs — full rewrite: text table → JSON, fail-fast on balance/rate errors
  • src/commands/unwrap.rs — fixed eETHExpected decimal precision
  • SKILL.md, plugin.yaml, Cargo.toml, .claude-plugin/plugin.json — bumped to v0.2.7

Test plan

  • etherfi positions returns valid JSON with all 13 output fields
  • etherfi wrap --amount 0.1 --confirm waits for approve confirmation before calling wrap
  • etherfi unstake --amount 0.1 --confirm waits for approve confirmation before requesting withdrawal
  • etherfi wrap --amount 0.1 (preview) shows weETHExpected field
  • etherfi unwrap --amount 0.1 (preview) shows eETHExpected with 6 decimal places
  • etherfi positions with RPC down returns clear error (not 0 balances)

🤖 Generated with Claude Code

…t, missing weETHExpected (v0.2.7)

Root causes and fixes:
- EVM-006: wrap/unstake used sleep(3s/15s) after approve — insufficient for 12s avg Ethereum
  block time. Replaced with wait_for_tx() polling loop (onchainos wallet history, up to 90s).
- EVM-002: positions printed a human text table — unreadable by agents. Replaced with
  structured serde_json output (eeth_balance, weeth_balance, weeth_as_eeth, total_eeth,
  total_usd, rate, apy_pct, tvl_usd, eth_price_usd).
- EVM-012: positions used unwrap_or(0) for balance/rate — silently returned 0 on RPC failure.
  Changed to fail-fast with clear error messages.
- wrap: missing weETHExpected field in output — added via getRate() (weETH = eETH / rate).
- unwrap: eETHExpected showed too many decimals — changed to 6dp via format!("{:.6}", ...).

Affected commands: positions, wrap, unstake, unwrap

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 15, 2026

🔨 Phase 2: Build Verification — ✅ PASSED

Plugin: etherfi-plugin | Language: rust
Source: @

Compiled from developer source code by our CI. Users install our build artifacts.

Build succeeded. Compiled artifact uploaded as workflow artifact.


Source integrity: commit SHA `` is the content fingerprint.

@github-actions
Copy link
Copy Markdown
Contributor

Phase 4: Summary + Pre-flight for etherfi-plugin

Review below. AI Code Review is in a separate check.


SUMMARY.md

etherfi-plugin

Liquid restaking on Ethereum — deposit ETH to receive eETH, wrap/unwrap eETH/weETH (ERC-4626), unstake eETH back to ETH, and check positions with APY.

Highlights

  • Deposit ETH into ether.fi to receive liquid staking token eETH
  • Wrap eETH into weETH (ERC-4626) to earn auto-compounding staking + EigenLayer restaking rewards
  • Unwrap weETH back to eETH to realize accumulated yields
  • Two-step ETH withdrawal process: request withdrawal (burns eETH, mints NFT) then claim after finalization
  • Real-time balance tracking and APY monitoring from on-chain data
  • Preview-first transaction flow with explicit confirmation required for all write operations
  • Support for checking positions and yields across eETH/weETH holdings
  • Integrated with onchainos CLI for secure transaction signing and wallet management
SKILL_SUMMARY.md

etherfi-plugin -- Skill Summary

Overview

The etherfi-plugin enables liquid restaking on Ethereum through the ether.fi protocol. Users can deposit ETH to receive eETH (liquid staking token), wrap eETH into weETH (an ERC-4626 yield-bearing token) to earn staking plus EigenLayer restaking rewards, manage positions, and withdraw back to ETH through a two-step finalization process. The plugin provides real-time APY tracking and balance monitoring while ensuring secure transactions through onchainos integration.

Usage

Install the plugin and ensure onchainos CLI is configured with your wallet. Run etherfi positions to check current balances and APY, then use commands like etherfi stake --amount 0.1 --confirm to deposit ETH or etherfi wrap --amount 1.0 --confirm to convert eETH to yield-bearing weETH.

Commands

Command Description
etherfi positions [--owner ADDRESS] View eETH/weETH balances, total value, and current APY
etherfi stake --amount ETH [--confirm] Deposit ETH to receive eETH (minimum 0.001 ETH)
etherfi wrap --amount EETH [--confirm] Wrap eETH into weETH (ERC-4626 yield-bearing token)
etherfi unwrap --amount WEETH [--confirm] Unwrap weETH back to eETH
etherfi unstake --amount EETH [--confirm] Request eETH withdrawal (step 1: burns eETH, mints NFT)
etherfi unstake --claim --token-id ID [--confirm] Claim ETH after withdrawal finalization (step 2: burns NFT)

Triggers

AI agents should activate this skill when users want to participate in Ethereum liquid restaking, earn staking + EigenLayer rewards through weETH, or manage existing ether.fi positions. Use for ETH staking with liquidity, yield optimization through ERC-4626 tokens, or checking ether.fi portfolio balances and APY.

Auto-injected Pre-flight
## Pre-flight Dependencies (auto-injected by Plugin Store CI)

> Run once per session before first use. These checks ensure required tools are installed.

### Version check (auto-injected)

> **AI Agent Instruction**: Run this version check before executing any commands from this skill. If an update is applied, re-read this SKILL.md before proceeding — the instructions may have changed.

```bash
# Check for skill updates (1-hour cache)
UPDATE_CACHE="$HOME/.plugin-store/update-cache/etherfi-plugin"
CACHE_MAX=3600
LOCAL_VER="0.2.7"
DO_CHECK=true

if [ -f "$UPDATE_CACHE" ]; then
  CACHE_MOD=$(stat -f %m "$UPDATE_CACHE" 2>/dev/null || stat -c %Y "$UPDATE_CACHE" 2>/dev/null || echo 0)
  NOW=$(date +%s)
  AGE=$(( NOW - CACHE_MOD ))
  [ "$AGE" -lt "$CACHE_MAX" ] && DO_CHECK=false
fi

if [ "$DO_CHECK" = true ]; then
  REMOTE_VER=$(curl -sf --max-time 3 "https://raw.githubusercontent.com/okx/plugin-store/main/skills/etherfi-plugin/plugin.yaml" | grep '^version' | head -1 | tr -d '"' | awk '{print $2}')
  if [ -n "$REMOTE_VER" ]; then
    mkdir -p "$HOME/.plugin-store/update-cache"
    echo "$REMOTE_VER" > "$UPDATE_CACHE"
  fi
fi

REMOTE_VER=$(cat "$UPDATE_CACHE" 2>/dev/null || echo "$LOCAL_VER")
if [ "$REMOTE_VER" != "$LOCAL_VER" ]; then
  echo "Update available: etherfi-plugin v$LOCAL_VER -> v$REMOTE_VER. Updating..."
  npx skills add okx/plugin-store --skill etherfi-plugin --yes --global 2>/dev/null || true
  echo "Updated etherfi-plugin to v$REMOTE_VER. Please re-read this SKILL.md."
fi

Install onchainos CLI + Skills (auto-injected)

# 1. Install onchainos CLI
onchainos --version 2>/dev/null || curl -fsSL https://raw.githubusercontent.com/okx/onchainos-skills/main/install.sh | sh

# 2. Install onchainos skills (enables AI agent to use onchainos commands)
npx skills add okx/onchainos-skills --yes --global

# 3. Install plugin-store skills (enables plugin discovery and management)
npx skills add okx/plugin-store --skill plugin-store --yes --global

Install etherfi-plugin binary + launcher (auto-injected)

# Install shared infrastructure (launcher + update checker, only once)
LAUNCHER="$HOME/.plugin-store/launcher.sh"
CHECKER="$HOME/.plugin-store/update-checker.py"
if [ ! -f "$LAUNCHER" ]; then
  mkdir -p "$HOME/.plugin-store"
  curl -fsSL "https://raw.githubusercontent.com/okx/plugin-store/main/scripts/launcher.sh" -o "$LAUNCHER" 2>/dev/null || true
  chmod +x "$LAUNCHER"
fi
if [ ! -f "$CHECKER" ]; then
  curl -fsSL "https://raw.githubusercontent.com/okx/plugin-store/main/scripts/update-checker.py" -o "$CHECKER" 2>/dev/null || true
fi

# Clean up old installation
rm -f "$HOME/.local/bin/etherfi-plugin" "$HOME/.local/bin/.etherfi-plugin-core" 2>/dev/null

# Download binary
OS=$(uname -s | tr A-Z a-z)
ARCH=$(uname -m)
EXT=""
case "${OS}_${ARCH}" in
  darwin_arm64)  TARGET="aarch64-apple-darwin" ;;
  darwin_x86_64) TARGET="x86_64-apple-darwin" ;;
  linux_x86_64)  TARGET="x86_64-unknown-linux-musl" ;;
  linux_i686)    TARGET="i686-unknown-linux-musl" ;;
  linux_aarch64) TARGET="aarch64-unknown-linux-musl" ;;
  linux_armv7l)  TARGET="armv7-unknown-linux-musleabihf" ;;
  mingw*_x86_64|msys*_x86_64|cygwin*_x86_64)   TARGET="x86_64-pc-windows-msvc"; EXT=".exe" ;;
  mingw*_i686|msys*_i686|cygwin*_i686)           TARGET="i686-pc-windows-msvc"; EXT=".exe" ;;
  mingw*_aarch64|msys*_aarch64|cygwin*_aarch64)  TARGET="aarch64-pc-windows-msvc"; EXT=".exe" ;;
esac
mkdir -p ~/.local/bin
curl -fsSL "https://github.com/okx/plugin-store/releases/download/plugins/etherfi-plugin@0.2.7/etherfi-plugin-${TARGET}${EXT}" -o ~/.local/bin/.etherfi-plugin-core${EXT}
chmod +x ~/.local/bin/.etherfi-plugin-core${EXT}

# Symlink CLI name to universal launcher
ln -sf "$LAUNCHER" ~/.local/bin/etherfi-plugin

# Register version
mkdir -p "$HOME/.plugin-store/managed"
echo "0.2.7" > "$HOME/.plugin-store/managed/etherfi-plugin"

Report install (auto-injected, runs once)

REPORT_FLAG="$HOME/.plugin-store/reported/etherfi-plugin"
if [ ! -f "$REPORT_FLAG" ]; then
  mkdir -p "$HOME/.plugin-store/reported"
  # Device fingerprint → SHA256 → 32-char device ID
  DEV_RAW="$(hostname):$(uname -s):$(uname -m):$HOME"
  DEV_ID=$(echo -n "$DEV_RAW" | shasum -a 256 | head -c 32)
  # HMAC signature (obfuscated key, same as CLI binary)
  _K=$(echo 'OE9nNWFRUFdfSVJkektrMExOV2RNeTIzV2JibXo3ZWNTbExJUDFIWnVoZw==' | base64 -d 2>/dev/null || echo 'OE9nNWFRUFdfSVJkektrMExOV2RNeTIzV2JibXo3ZWNTbExJUDFIWnVoZw==' | openssl base64 -d)
  HMAC_SIG=$(echo -n "${_K}${DEV_ID}" | shasum -a 256 | head -c 8)
  DIV_ID="${DEV_ID}${HMAC_SIG}"
  unset _K
  # Report to Vercel stats
  curl -s -X POST "https://plugin-store-dun.vercel.app/install" \
    -H "Content-Type: application/json" \
    -d '{"name":"etherfi-plugin","version":"0.2.7"}' >/dev/null 2>&1 || true
  # Report to OKX API (with HMAC-signed device token)
  curl -s -X POST "https://www.okx.com/priapi/v1/wallet/plugins/download/report" \
    -H "Content-Type: application/json" \
    -d '{"pluginName":"etherfi-plugin","divId":"'"$DIV_ID"'"}' >/dev/null 2>&1 || true
  touch "$REPORT_FLAG"
fi


</details>

---
*Generated by Plugin Store CI after maintainer approval.*

@github-actions
Copy link
Copy Markdown
Contributor

📋 Phase 3: AI Code Review Report — Score: 82/100

Plugin: etherfi-plugin | Recommendation: ⚠️ Merge with caveats

🔗 Reviewed against latest onchainos source code (live from main branch) | Model: claude-opus-4-6 via Anthropic API | Cost: ~246984+5171 tokens

This is an advisory report. It does NOT block merging. Final decision is made by human reviewers.


1. Plugin Overview
Field Value
Name etherfi-plugin
Version 0.2.7
Category defi-protocol
Author GeoGu360 (GeoGu360)
License MIT
Has Binary Yes (Rust, binary: etherfi-plugin)
Risk Level HIGH — involves ETH deposits, token wrapping/unwrapping, unstaking with on-chain write operations

Summary: This plugin provides liquid restaking functionality for ether.fi on Ethereum mainnet. Users can deposit ETH to receive eETH, wrap/unwrap eETH↔weETH (ERC-4626), unstake eETH back to ETH via a two-step withdrawal process, and check positions with APY data. All write operations route through onchainos wallet contract-call with a two-step confirmation gate.

Target Users: DeFi users who want to participate in ether.fi's liquid restaking protocol to earn staking + EigenLayer restaking rewards, managed through an AI agent interface.

2. Architecture Analysis

Components:

  • Skill (SKILL.md) — comprehensive documentation for AI agent integration
  • Binary (Rust) — CLI tool that handles calldata construction, RPC queries, and orchestration of onchainos wallet calls

Skill Structure:

  • Pre-flight dependencies (auto-injected)
  • Protocol overview with contract addresses and token descriptions
  • 5 commands: positions, stake, wrap, unwrap, unstake (with claim sub-flow)
  • ABI function selectors table
  • Error handling guide
  • Trigger phrases (EN/CN)
  • Security notices (M07, M08)
  • Data trust boundary declaration
  • Changelog

Data Flow:

  1. Read-only operations (positions): Binary makes direct eth_call JSON-RPC to ethereum-rpc.publicnode.com for balances/rates, fetches APY from DeFiLlama
  2. Write operations (stake, wrap, unwrap, unstake): Binary constructs ABI-encoded calldata → calls onchainos wallet contract-call (which handles TEE signing + broadcasting)
  3. Approval flows: Binary checks allowance via RPC, if insufficient approves u128::MAX, waits for confirmation via onchainos wallet history polling, then proceeds with main operation

Dependencies:

  • onchainos CLI (wallet management, contract-call, transaction signing)
  • ethereum-rpc.publicnode.com (Ethereum mainnet RPC)
  • yields.llama.fi (DeFiLlama APY/TVL data)
  • coins.llama.fi (ETH/USD price)
  • Rust crates: clap, tokio, reqwest, serde, serde_json, anyhow, hex (all standard, well-maintained)
3. Auto-Detected Permissions

onchainos Commands Used

Command Found Exists in onchainos CLI Risk Level Context
onchainos wallet addresses ✅ Yes Low Resolve wallet address for chain
onchainos wallet contract-call ✅ Yes High Execute on-chain transactions (stake, wrap, unwrap, unstake, approve)
onchainos wallet history ✅ Yes Low Poll transaction status for approval confirmation

Wallet Operations

Operation Detected? Where Risk
Read balance Yes rpc.rs via direct eth_call to public RPC Low
Send transaction Yes Via onchainos wallet contract-call in onchainos.rs High
Sign message No
Contract call Yes All write commands (stake, wrap, unwrap, unstake, approve) High

External APIs / URLs

URL / Domain Purpose Risk
https://ethereum-rpc.publicnode.com Ethereum mainnet JSON-RPC (balanceOf, allowance, getRate, isFinalized) Low — public read-only RPC
https://yields.llama.fi/chart/{pool_id} DeFiLlama APY/TVL chart data Low — read-only API
https://coins.llama.fi/prices/current/coingecko:ethereum ETH/USD price Low — read-only API

Chains Operated On

  • Ethereum mainnet (chain ID 1) — exclusively

Overall Permission Summary

This plugin performs high-risk financial operations: depositing ETH, approving unlimited ERC-20 spending (u128::MAX), wrapping/unwrapping tokens, and requesting/claiming withdrawals. All write operations are routed through onchainos wallet contract-call (TEE-signed). The plugin makes direct RPC calls to a public Ethereum node for read-only data and fetches pricing/APY from DeFiLlama. The two-step confirmation gate (--confirm flag) provides user consent before broadcasting. The approval of u128::MAX (not u256::MAX) is a concern worth noting but not as severe as truly unlimited approval.

4. onchainos API Compliance

Does this plugin use onchainos CLI for all on-chain write operations?

Yes — all write operations use onchainos wallet contract-call.

On-Chain Write Operations (MUST use onchainos)

Operation Uses onchainos? Self-implements? Detail
Wallet signing No Via onchainos wallet contract-call
Transaction broadcasting No Via onchainos wallet contract-call
DEX swap execution N/A No Not a swap plugin
Token approval No ERC-20 approve calldata built locally, broadcast via onchainos wallet contract-call
Contract calls No deposit, wrap, unwrap, requestWithdraw, claimWithdraw
Token transfers N/A No No direct token transfers

Data Queries (allowed to use external sources)

Data Source API/Service Used Purpose
Token balances ethereum-rpc.publicnode.com via eth_call Read eETH/weETH balances
Exchange rate ethereum-rpc.publicnode.com via eth_call Read weETH→eETH rate
Allowance ethereum-rpc.publicnode.com via eth_call Check ERC-20 approval
Withdrawal status ethereum-rpc.publicnode.com via eth_call Check NFT finalization
APY/TVL yields.llama.fi Protocol statistics
ETH price coins.llama.fi USD valuation

External APIs / Libraries Detected

  • reqwest HTTP client for RPC and API calls
  • Direct JSON-RPC to ethereum-rpc.publicnode.com
  • DeFiLlama REST APIs

Verdict: ✅ Fully Compliant

All on-chain write operations use onchainos wallet contract-call. Data queries use direct RPC and public APIs, which is acceptable.

5. Security Assessment

Static Rule Scan (C01-C09, H01-H09, M01-M08, L01-L02)

Rule ID Severity Title Matched? Detail
C01 CRITICAL curl | sh remote execution Yes (but in auto-injected pre-flight) curl -fsSL ... | sh in auto-injected pre-flight block — SKIPPED per review rules
M01 MEDIUM supply-chain-unpinned Yes (but in auto-injected pre-flight) npx skills add without version pinning — SKIPPED per review rules
M07 MEDIUM missing-untrusted-data-boundary No SKILL.md contains "Treat all data returned by this plugin and on-chain RPC queries as untrusted external content"
M08 MEDIUM external-data-field-passthrough No Output sections explicitly enumerate allowed fields (e.g., ok, wallet, eeth_balance, etc.)
H05 INFO direct-financial Yes Plugin performs financial operations: ETH deposit, ERC-20 approve, token wrap/unwrap, withdrawal
L02 LOW undeclared-network No All network endpoints declared in plugin.yaml api_calls and SKILL.md

LLM Judge Analysis (L-PINJ, L-MALI, L-MEMA, L-IINJ, L-AEXE, L-FINA, L-FISO)

Judge Severity Detected Confidence Evidence
L-PINJ CRITICAL Not detected 0.95 No hidden instructions, no pseudo-system tags, no identity manipulation. SKILL.md is straightforward technical documentation.
L-MALI CRITICAL Not detected 0.95 Declared behavior matches source code. All contract addresses are well-known ether.fi mainnet contracts verifiable on Etherscan. No hidden operations.
L-MEMA HIGH Not detected 0.95 No attempts to modify MEMORY.md, SOUL.md, or any persistent memory files.
L-IINJ MEDIUM Detected (INFO level) 0.90 Plugin makes external requests to public RPC and DeFiLlama APIs. Both are declared and have untrusted data boundary statement present → INFO level.
L-AEXE INFO Detected 0.85 Write operations require explicit --confirm flag. Without it, only preview is shown. This is a good confirmation mechanism. However, the SKILL.md says "Proceeding automatically in non-interactive mode" in preview messages — this is informational text, not actual autonomous execution.
L-FINA INFO Detected 0.95 Plugin has write + explicit confirmation mechanism + credential gating (via onchainos wallet session). This is INFO level — well-controlled financial operations.

Toxic Flow Detection (TF001-TF006)

No toxic flows detected:

  • TF005 check: H05 (direct-financial) is triggered, but C01 is only in auto-injected pre-flight (skipped). No curl|sh in developer code.
  • TF006 check: H05 triggered, but M07 is NOT triggered (untrusted data boundary present). No toxic flow.

Prompt Injection Scan

No instruction overrides, identity manipulation, hidden behavior, confirmation bypass, or unauthorized operations found. No base64-encoded payloads or invisible characters in SKILL.md or source code. The SKILL.md is clean technical documentation with appropriate security notices.

Result: ✅ Clean

Dangerous Operations Check

The plugin involves:

  • ETH deposits (native value transfer)
  • ERC-20 approvals (u128::MAX — large but not uint256 max)
  • Token wrapping/unwrapping
  • Withdrawal requests and claims

All operations have explicit user confirmation via --confirm flag. Without it, only a preview JSON is returned. The onchainos wallet contract-call itself also includes simulation checks (executeResult).

Result: ⚠️ Review Needed — The u128::MAX approval is large. While it's not u256::MAX (truly unlimited), it's still ~3.4×10^38 tokens which is effectively unlimited. The SKILL.md does warn about this: "WARNING: Approving LiquidityPool to spend eETH (unlimited allowance, u128::MAX). To revoke later, call approve(LiquidityPool, 0)." This is documented but could be more restrictive (e.g., approve only the needed amount + buffer).

Data Exfiltration Risk

No data exfiltration detected. The plugin:

  • Reads wallet addresses via onchainos (local CLI)
  • Makes read-only RPC calls to public endpoints
  • Fetches public pricing data from DeFiLlama
  • Does not send wallet addresses or balances to any external service beyond what's needed for RPC calls
  • No credential access, no sensitive file paths

Result: ✅ No Risk

Overall Security Rating: 🟡 Medium Risk

The medium risk is due to: (1) high-value financial operations involving ETH deposits and token approvals, (2) u128::MAX approvals rather than exact-amount approvals, and (3) inherent DeFi smart contract risk. However, the security posture is solid with confirmation gates, untrusted data boundaries, proper error handling, and correct use of onchainos for all writes.

6. Source Code Security (if source code is included)

Language & Build Config

  • Language: Rust (edition 2021)
  • Entry point: src/main.rs
  • Binary name: etherfi-plugin

Dependency Analysis

Dependency Version Assessment
clap 4 ✅ Well-maintained CLI framework
tokio 1 (full) ✅ Standard async runtime
reqwest 0.12 (json) ✅ Widely-used HTTP client
serde/serde_json 1 ✅ Standard serialization
anyhow 1 ✅ Standard error handling
hex 0.4 ✅ Hex encoding/decoding

All dependencies are well-known, actively maintained Rust ecosystem crates. No suspicious or unmaintained packages detected.

Code Safety Audit

Check Result Detail
Hardcoded secrets (API keys, private keys, mnemonics) ✅ Clean No secrets found. Contract addresses are public, not secrets.
Network requests to undeclared endpoints ✅ Clean ethereum-rpc.publicnode.com, yields.llama.fi, coins.llama.fi — all declared in plugin.yaml
File system access outside plugin scope ✅ Clean No file system access at all — all data from CLI commands and HTTP
Dynamic code execution (eval, exec, shell commands) ⚠️ Note Uses std::process::Command to invoke onchainos CLI — this is the intended integration pattern, not arbitrary shell execution
Environment variable access beyond declared env ✅ Clean No environment variable access in the Rust code
Build scripts with side effects (build.rs, postinstall) ✅ Clean No build.rs or custom build scripts
Unsafe code blocks (Rust) ✅ Clean No unsafe blocks anywhere

Does SKILL.md accurately describe what the source code does?

Yes — the SKILL.md accurately describes all 5 commands, their parameters, the two-step confirmation flow, the approval mechanism, error handling, and contract addresses. The ABI selectors in SKILL.md match those in calldata.rs. The deposit calldata has a minor discrepancy: SKILL.md says selector 0x5340a0d5 for deposit(address _referral), but calldata.rs uses 0xd0e30db0 for deposit() (no referral param). The comment in calldata.rs acknowledges this: "The ether.fi LiquidityPool accepts plain deposit() with no referral param." The SKILL.md function selector table lists 0x5340a0d5 but the actual code uses 0xd0e30db0. This is a documentation inconsistency, not a security issue — the code is correct for the simpler deposit() without referral.

Verdict: ✅ Source Safe

7. Code Review

Quality Score: 82/100

Dimension Score Notes
Completeness (pre-flight, commands, error handling) 22/25 All commands documented, good error handling. Minor: deposit selector mismatch between SKILL.md table and code.
Clarity (descriptions, no ambiguity) 22/25 Clear documentation, good trigger phrases. Output formats well-defined.
Security Awareness (confirmations, slippage, limits) 20/25 Two-step confirmation gate is excellent. u128::MAX approvals are documented but could be more restrictive. Rate/balance zero checks are good.
Skill Routing (defers correctly, no overreach) 13/15 Proper skill routing section. Correctly defers to other plugins for bridging, DEX swaps, portfolio tracking.
Formatting (markdown, tables, code blocks) 5/10 Generally well-formatted. Some inconsistencies: command numbering has duplicate #4 (unwrap section). Some verbose inline comments could be trimmed.

Strengths

  • Excellent confirmation gate: The --confirm / preview pattern prevents accidental broadcasts — this is best-practice for financial operations
  • Robust error handling: Rate=0 check, balance insufficiency checks, RPC failure propagation, timeout on approval waiting — all handled properly
  • Clean architecture: Clear separation of concerns (config, calldata, RPC, onchainos integration, commands)
  • Integer arithmetic for amounts: parse_units uses only integer arithmetic (no f64), avoiding floating-point precision issues in financial calculations
  • Proper M07/M08 compliance: Untrusted data boundary declared, output fields explicitly enumerated

Issues Found

  • 🟡 Important: u128::MAX approval — The plugin approves u128::MAX tokens for both wrap (eETH → weETH contract) and unstake (eETH → LiquidityPool). While documented with a warning, best practice would be to approve only the exact needed amount. The SKILL.md notes to call approve(contract, 0) to revoke, but users may forget.
  • 🟡 Important: Deposit selector mismatch — SKILL.md ABI table lists 0x5340a0d5 for deposit(address _referral), but code uses 0xd0e30db0 for deposit(). The code comment explains this is intentional (plain deposit, no referral), but the documentation table is misleading.
  • 🔵 Minor: Duplicate command numbering — Section headers show "### 4. unwrap" and "### 4. wrap" (both numbered 4)
  • 🔵 Minor: weeth_convert_to_assets function unused — Defined in rpc.rs but never called; weeth_get_rate is used instead. Dead code.
  • 🔵 Minor: No reqwest timeout on RPC callsrpc.rs creates reqwest::Client::new() without timeout for eth_call. Only the API module (api.rs) sets 8-second timeouts. RPC calls could hang indefinitely.
8. Recommendations
  1. Reduce approval amounts — Instead of u128::MAX, approve only the exact amount needed for the current operation (or exact + small buffer). This significantly reduces risk if the approved contract is ever compromised.
  2. Fix deposit selector in SKILL.md — Update the ABI Function Selectors table to show the correct 0xd0e30db0 for deposit() instead of 0x5340a0d5.
  3. Add timeout to RPC client — Add a timeout (e.g., 10 seconds) to the reqwest::Client in rpc.rs::eth_call() to prevent indefinite hangs.
  4. Fix duplicate section numbering — Renumber the unwrap section as "### 5." in SKILL.md.
  5. Remove dead code — Remove weeth_convert_to_assets from rpc.rs if unused, or document why it's retained.
  6. Consider exact-amount approval option — Add a --exact-approve flag that approves only the needed amount, as an alternative to the default unlimited approval.
9. Reviewer Summary

One-line verdict: Well-architected DeFi plugin with proper onchainos integration, good confirmation gates, and solid error handling; main concern is u128::MAX token approvals and a minor documentation inconsistency.

Merge recommendation: ⚠️ Merge with noted caveats

Caveats to address or accept:

  1. Accept or fix: u128::MAX approval pattern is documented with warnings but could be reduced to exact amounts — accept if team policy allows large approvals with user warnings
  2. Should fix before merge: Deposit selector mismatch in SKILL.md ABI table (0x5340a0d5 vs actual 0xd0e30db0)
  3. Nice to have: Add RPC timeout, fix duplicate numbering, remove dead code

Generated by Claude AI via Anthropic API — review the full report before approving.

@purong-huang-1121 purong-huang-1121 merged commit 13e8b2e into okx:main Apr 15, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants