Conversation
Code Review ReportProject: sun-cli PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR is a documentation-only change — it expands and reorganizes The overall quality of the documentation expansion is high: the new reference structure is clear, consistent, and significantly more useful for developers and AI agents consuming the CLI. The addition of a However, one Major issue stands out: the Change Summary1. Table of Contents Restructuring
Purpose: The old "Command Guide" with broad groupings (e.g., "Price & Discovery", "Protocol & History") is replaced by a flat "Command Reference" where each logical command group gets its own ToC entry. This improves navigability and discoverability. 2. Per-Command Option Tables
Purpose: The old documentation used code-block snippets only, showing example invocations without explaining individual options. The new format documents each option's type, description, and default value — critical for AI-agent consumers and new developers. 3. New Sections: Pool, Position, Pair, Farm, Transaction
Purpose: Commands previously buried in "Price & Discovery" and "Protocol & History" omnibus sections are elevated to first-class reference sections. 4. Built-In Token Symbol Table Update
Purpose: Reflects a token symbol update. The padding removal is a style normalization. Detailed FindingsMajor[MJ-01]
|
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Documentation |
| File | README.md : Built-In Token Symbols table (approx. lines 948–960 in update) |
Description
The built-in token symbol USDC has been renamed to USDCOLD in the token symbol reference table without any changelog entry, deprecation notice, or migration guidance. Any user or automation script that currently calls sun price USDC, sun swap TRX USDC ..., or similar commands based on the documented symbol will receive an error or unexpected behavior after updating. The TRON contract address (TEkxiTehnzSmSe2XqrBj4w32RUN966rdz8) remains the same, so the change is a symbol alias rename, not an address correction.
Code
-| `USDC` | `TEkxiTehnzSmSe2XqrBj4w32RUN966rdz8` | 6 |
+| `USDCOLD` | `TEkxiTehnzSmSe2XqrBj4w32RUN966rdz8` | 6 |Recommendation
- If
USDCOLDis already live in the CLI code, add a migration notice near the symbol table:> **Note (v1.x → v2.x):** The `USDC` symbol has been renamed to `USDCOLD` to reflect > the legacy contract. Users should update scripts accordingly. The new canonical > USDC (native bridge) address is `<new address if applicable>`.
- Confirm whether the actual
sun-kit/ CLI source code has been updated to match; if not, this documentation change is inconsistent with the implementation. - Consider keeping
USDCas a deprecated alias (still accepted) with a warning message, to avoid a hard breakage.
Minor
[MN-01] position list and position tick Documented Twice
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation / Code Quality |
| File | README.md : "Wallet & Portfolio" section AND "Position" section |
Description
Both position list and position tick <poolAddress> are documented in full (including option tables) under two separate sections: once under "Wallet & Portfolio" and again under the dedicated "Position" section. This duplication creates a maintenance burden — future changes to these commands will require updating two places — and may confuse readers about which reference is authoritative.
Code
### Wallet & Portfolio
...
#### `position list` (read)
...
#### `position tick <poolAddress>` (read)
...
### Position
...
#### `position list` (read)
...
#### `position tick <poolAddress>` (read)Recommendation
Remove the position list and position tick sub-sections from "Wallet & Portfolio". Replace them with a cross-reference:
### Wallet & Portfolio
#### `wallet address` (read)
...
#### `wallet balances` (read)
...
> For liquidity positions, see [Position](#position).
> For farming positions, see [Farm](#farm).[MN-02] Misleading Contract Address in contract send Example
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation / Correctness |
| File | README.md : contract send section |
Description
The example for contract send uses TRecipient as both the contract address argument and the transfer recipient inside --args, which is confusing. TRecipient is a placeholder name suggesting a person's wallet address, not a smart contract address. The first positional argument to contract send is always a smart contract address; using the same placeholder for both reinforces a common misunderstanding that could lead users to send transactions to the wrong address.
Code
sun contract send TRecipient transfer --args '["TRecipient","1000000"]' --value 0Recommendation
Use a clearly distinct placeholder for the contract address:
# Transfer tokens via a TRC-20 contract
sun contract send TR7NHqjeKQxGTCi8q8ZY4pL8otSzgjLj6t transfer \
--args '["TRecipientAddress","1000000"]' --value 0Or use the same pattern as contract read, which correctly uses a real contract address:
sun contract read TR7NHqjeKQxGTCi8q8ZY4pL8otSzgjLj6t balanceOf --args '["TYourAddress"]'[MN-03] farm tx --type Does Not Enumerate Valid Values
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | README.md : farm tx section |
Description
The --type <farmTxType> option for farm tx uses the placeholder type name farmTxType without enumerating valid values. By contrast, the analogous tx scan --type option clearly documents the valid values as swap, add, withdraw. Users and AI agents consuming the farm tx command cannot determine valid inputs from the documentation alone.
Code
| `--type <farmTxType>` | Transaction type | — |Recommendation
Enumerate the valid values in the description column, e.g.:
| `--type <farmTxType>` | Transaction type: `stake`, `unstake`, `harvest`, ... | — |(Confirm the exact valid enum values from the CLI source at src/commands/farm/tx.ts or similar.)
[MN-04] liquidity v4:collect Missing --recipient Option
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | README.md : liquidity v4:collect section |
Description
liquidity v3:collect documents a --recipient <address> option (defaulting to the active wallet), but liquidity v4:collect does not document any equivalent option. If the underlying V4 collect command accepts a recipient address, this omission leaves users unable to direct fees to a different address via the documented interface. If V4 collect genuinely has no recipient option, a brief note to that effect would prevent confusion.
Code
#### `liquidity v3:collect` (write)
| `--recipient <address>` | Fee recipient | Active wallet | ← documented ✓
#### `liquidity v4:collect` (write)
| `--token-id <id>` | ...
| `--token0 ...` | ...
| `--token1 ...` | ...
| `--fee <n>` | ...
| `--deadline ...` | ...
← no --recipientRecommendation
Either add --recipient <address> to v4:collect if supported:
| `--recipient <address>` | Fee recipient | Active wallet |Or add a note explaining the V4 design difference:
> **Note:** V4 collects always direct fees to the position owner. Use `--token0`, `--token1`, and `--fee` to identify the pool.Suggestions
[S-01] v4:mint — Clarify When --sqrt-price and --fee Are Required
File: README.md : liquidity v4:mint section
Description: --sqrt-price is described as "Initial sqrtPriceX96 (for pool creation)" and --create-pool has a false default, but there is no note that --sqrt-price is required when --create-pool is set. Similarly, --fee has no default value listed, making it unclear if it is optional or effectively required. V3:mint has --fee defaulting to 3000; the absence of a default for V4 may be intentional (hooks may determine fee) but should be explained.
Suggestion: Add an inline note below the V4 mint table:
> When `--create-pool` is set, `--sqrt-price` is required to initialize the pool's price.
> `--fee` defaults to the hook configuration; supply explicitly for fee-tier pools.[S-02] Global Flags Table Lost Column Alignment
File: README.md : "Global Flags" section
Description: The old table used padded column alignment for readability in source (raw Markdown). The new table removes all padding, making the source harder to read and diff. This is a style preference but impacts maintainability.
Suggestion: Restore the padded source alignment, or ensure the rendering target (GitHub) is the primary concern (in which case the current form is acceptable since GitHub renders both identically).
[S-03] No Write Operations Documented for Farm Commands
File: README.md : "Farm" section
Description: The Farm section only documents read-only commands (farm list, farm tx, farm positions). If the CLI supports staking, unstaking, or harvesting rewards (write operations), these are absent from the new reference. The old documentation had the same gap, so this is not a regression — but the PR is an opportunity to close it.
Suggestion: If write operations exist in the codebase, add them to this section following the same pattern used for liquidity commands. If they are planned, add a placeholder note:
> Write operations (stake, unstake, harvest) are not yet documented.Positive Observations
| Area | Observation |
|---|---|
| Argument Annotations | The <arg> vs [arg] convention with a legend at the top of "Command Reference" is a clear UX improvement for both humans and AI agents. |
**(required)** Marker |
Marking mandatory options inline within tables (e.g., --router, --token-a, --token-id) prevents common CLI misuse errors and avoids burying critical info in prose. |
(read) / (write) Labels |
Annotating each sub-command as read-only or write allows users to quickly assess safety before execution — especially valuable for AI-agent consumers. |
| Default Value Column | Every option table now includes a "Default" column. This eliminates ambiguity about what happens when an option is omitted and matches the quality bar of tools like kubectl --help. |
| Single-Sided Liquidity Notes | Inline notes for V3 mint/increase explaining auto-calculation behavior when only one token amount is provided are accurate and reduce support friction. |
| Section Granularity | Elevating Pool, Farm, Pair, Position, and Transaction to top-level sections (with their own ToC entries) significantly improves discoverability versus the previous "catch-all" groupings. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 3 | 2 | 1 | 5 | MJ-01: USDC→USDCOLD may be inconsistent with code; MN-02: misleading example |
| Security | 8 | 8 | 0 | 0 | No credentials or sensitive data introduced |
| Performance | 7 | 0 | 0 | 7 | Documentation only — not applicable |
| Code Quality | 6 | 4 | 2 | 0 | MN-01: duplication; MN-03: incomplete enum |
| Testing | 5 | 0 | 0 | 5 | Documentation only — not applicable |
| Documentation | 6 | 4 | 2 | 0 | MN-04: missing v4:collect recipient; S-01: unclear v4:mint conditions |
| Compatibility | 5 | 3 | 1 | 1 | MJ-01: USDC symbol rename is a breaking change |
| Observability | 4 | 0 | 0 | 4 | Documentation only — not applicable |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analyzed only the diff between main and update. Since this PR modifies only README.md, no runtime behavior, source logic, or test coverage is affected — all findings relate to documentation accuracy, completeness, and consistency.
Report generated by Code Review Skill v1.0.0
Date: 2026-04-07
Code Review ReportProject: sun-cli PR OverviewBranch Information
Commit History
Review SummaryVerdict
Findings at a Glance
SummaryThis PR is a pure documentation update to Despite being documentation-only, the changes introduce two breaking behavioural issues that must be addressed before merge: a silent swap of the Change Summary1 — Table of Contents Restructure
Purpose: Align the ToC with the new per-command documentation structure and make individual command groups directly addressable. 2 — Command Reference Expansion
Purpose: Provide AI agents and human operators with complete, machine-parseable option metadata. 3 — Built-in Token Symbol Table Changes
Purpose: Reflect token contract migrations on the TRON network (USDC deprecated, USDD v2 migration). Detailed FindingsMajor[MJ-01] Silent
|
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Documentation |
| File | README.md : Built-in Token Symbols table |
Description
The
USDDsymbol has been silently reassigned fromTPYmHEhy5n8TCEfYGqW2rPxsghSfzghPDn(the original USDD contract) toTXDk8mbtRbXeYuMNS83CfKPaYYT8XWv9Hz(a different contract). The original address is now listed asUSDDOLD.Any user or AI agent with an existing workflow that runs
sun swap USDD ...will now execute the swap against a completely different on-chain token without any warning. In a DeFi CLI context this is a high-impact correctness issue: funds could be swapped into an unintended token, or liquidity could be added to the wrong pool. There is no changelog entry, deprecation notice, or migration guide for this change.
Code
-| `USDD` | `TPYmHEhy5n8TCEfYGqW2rPxsghSfzghPDn` | 18 |
+| `USDDOLD` | `TPYmHEhy5n8TCEfYGqW2rPxsghSfzghPDn` | 18 |
+| `USDD`| `TXDk8mbtRbXeYuMNS83CfKPaYYT8XWv9Hz` | 18 |Recommendation
<!-- Add a callout immediately above the token table, e.g.: -->
> **Breaking change (token symbols):** `USDD` now resolves to the migrated
> contract `TXDk8mbtRbXeYuMNS83CfKPaYYT8XWv9Hz`. The original contract is
> still accessible as `USDDOLD`. Update any existing scripts that reference
> `USDD` to verify they target the intended contract. `USDC` has similarly
> been renamed to `USDCOLD`; there is currently no new `USDC` alias.Also consider a semver minor/major bump and a CHANGELOG entry so automation can gate on the version.
[MJ-02] USDC Symbol Removed Without Deprecation Notice
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Documentation |
| File | README.md : Built-in Token Symbols table |
Description
The
USDCsymbol (TEkxiTehnzSmSe2XqrBj4w32RUN966rdz8) has been renamed toUSDCOLDwith no replacementUSDCalias and no deprecation warning. Existing scripts usingsun swap USDC ...,sun liquidity v3:mint --token0 USDC ..., etc., will now fail at runtime with an unknown symbol error. Users have no in-documentation signal that this is a breaking change.
Code
-| `USDC` | `TEkxiTehnzSmSe2XqrBj4w32RUN966rdz8` | 6 |
+| `USDCOLD` | `TEkxiTehnzSmSe2XqrBj4w32RUN966rdz8` | 6 |Recommendation
<!-- Option A — Add a migration callout (same callout as MJ-01 covers this) -->
<!-- Option B — Add a new USDC alias pointing to the current USDC contract
if one exists on TRON mainnet, e.g.: -->
| `USDC` | `<new_USDC_contract_address>` | 6 |
| `USDCOLD` | `TEkxiTehnzSmSe2XqrBj4w32RUN966rdz8` | 6 |[MJ-03] contract send Example Uses Recipient as Contract Address
| Property | Value |
|---|---|
| Severity | Major |
| Category | Correctness / Documentation |
| File | README.md : contract send section |
Description
The updated example for
contract sendincorrectly usesTRecipientas the contract address argument — the same value that appears as the transfer recipient inside--args. In the originalmainbranch the placeholder was the unambiguous<contractAddress>. The replacement makes the example self-contradictory: the contract being called appears to be a recipient wallet, not a TRC-20 token contract. This will confuse users and AI agents attempting to synthesise calls from the documentation.
Code
-sun contract send <contractAddress> transfer --args '["TRecipient","1000000"]' --value 0
+sun contract send TRecipient transfer --args '["TRecipient","1000000"]' --value 0Recommendation
# Use the canonical USDT contract address as a concrete, realistic example:
sun contract send TR7NHqjeKQxGTCi8q8ZY4pL8otSzgjLj6t transfer \
--args '["TRecipientAddress","1000000"]' --value 0Or retain the unambiguous angle-bracket placeholder from main:
sun contract send <contractAddress> transfer --args '["TRecipientAddress","1000000"]' --value 0Minor
[MN-01] position list / position tick Documented Twice
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation / Code Quality |
| File | README.md : "Wallet & Portfolio" section and "Position" section |
Description
position listandposition tickare documented in full (with option tables) both inside the "Wallet & Portfolio" section (lines ≈215–243) and again in the dedicated "Position" section (lines ≈669–691). The two occurrences are near-identical, creating maintenance duplication and reader confusion about which section is canonical.
Recommendation
<!-- In "Wallet & Portfolio", replace the duplicated full documentation
with a short cross-reference: -->
#### `position list` / `position tick` (read)
See [Position](#position) for full option reference.[MN-02] --no-blacklist Flag Description Is Ambiguous
| Property | Value |
|---|---|
| Severity | Minor |
| Category | Documentation |
| File | README.md : token list and pool list option tables |
Description
Both
token listandpool listoption tables describe--no-blacklistas "Include blacklisted tokens" with a default offalse. The phrasing is ambiguous: doesfalsemean the flag is off (blacklist enforced) or that blacklisted tokens are not included? The semantics of a--no-*boolean flag and afalsedefault need clearer wording to avoid misuse.
Recommendation
<!-- Replace the description with: -->
| `--no-blacklist` | Disable blacklist filtering — show all tokens, including blacklisted ones | `false` (blacklist enforced by default) |Suggestions
[S-01] Tokenlist Link Points to Legacy justswap.io Domain
File: README.md — bottom of "Built-In Token Symbols" section
Description: The newly added tokenlist hyperlink (https://list.justswap.io/justswap.json) uses the old justswap.io branding, while the host UI is served from sunlists.justnetwork.io. If the old domain is deprecated or moves, the JSON source will silently break. Consider hosting or mirroring the reference under the sun.io / justnetwork.io domain for long-term stability.
Suggestion: Verify the justswap.io JSON endpoint has an SLA / redirect guarantee, or replace with a canonical sunlists.justnetwork.io direct JSON URL if available.
Positive Observations
| Area | Observation |
|---|---|
| Documentation depth | The shift from terse one-liners to full option tables with types, descriptions, and defaults is a significant DX improvement for both human operators and AI agents. |
| Read/write annotations | Annotating each command as (read) or (write) provides clear, scannable safety cues — users can immediately see which operations require credentials and which are side-effect-free. |
| Required option callouts | Consistently marking required options with (required) in the description column eliminates guesswork and reduces runtime errors. |
| Single-sided liquidity notes | The v3:mint and v3:increase callout blocks explaining single-sided auto-calculation behaviour are precise and actionable. |
| Global Flags table reformatting | Normalising column widths in the Global Flags table (removing over-padded spacing) improves Markdown source readability without affecting rendered output. |
| Legend added | The <arg> / [arg] legend at the top of Command Reference sets clear expectations for the notation used throughout. |
Checklist Results
| Category | Items Checked | Pass | Fail | N/A | Notes |
|---|---|---|---|---|---|
| Correctness | 5 | 3 | 2 | 0 | USDD address swap, USDC symbol removal break existing users |
| Security | 3 | 3 | 0 | 0 | No credentials, no code paths — docs only |
| Performance | 1 | 1 | 0 | 0 | N/A for docs; pagination defaults documented correctly |
| Code Quality | 4 | 2 | 2 | 0 | Duplicate position docs; misleading contract send example |
| Testing | 2 | 2 | 0 | 0 | N/A — documentation change only |
| Documentation | 6 | 4 | 2 | 0 | --no-blacklist ambiguity; missing breaking-change callout |
| Compatibility | 3 | 1 | 2 | 0 | Two built-in symbol renames are undocumented breaking changes |
| Observability | 1 | 1 | 0 | 0 | N/A for docs |
Disclaimer
This is an automated code review. It supplements but does not replace human review. The reviewer analysed only the diff between main and update. Runtime behaviour, on-chain contract verification, and deployment impact are not covered. Contract addresses listed in the diff were not independently verified on-chain — confirm TXDk8mbtRbXeYuMNS83CfKPaYYT8XWv9Hz is the correct canonical USDD v2 address before shipping.
Report generated by Code Review Skill v1.0.0
Date: 2026-04-07
2 participants
No description provided.