Repository navigation
docs: use the USDC token address in the fee-currency CLI example, checksum USDT, and link the DEK page directly - #2366
Conversation
GigaHierz
left a comment
There was a problem hiding this comment.
head cec841c · API head == refs/pull/2366 (asserted); 1 commit, 2 files, 1 commit behind main (a12466b); git merge-tree --write-tree origin/main clean; on a temp merge into main mint broken-links --check-redirects no broken links and check-orphans.sh no orphan pages; gh pr checks: broken-links pass, GitGuardian pass, StepSecurity pass, Mintlify Deployment skipping; checksums by cast to-check-sum-address, symbol()/decimals()/eth_getCode and FeeCurrencyDirectory.getCurrencies() on forno.celo.org; CLI help run from @celo/celocli@9.0.1
Verdict: REQUEST-CHANGES
Findings
-
MEDIUM ·
build-on-celo/fee-abstraction/using-fee-abstraction.mdx:46-56(the USDC block directly above the lines this PR edits) · The firstcelocli transfer:erc20example, the one that guide section opens with, passes the USDC adapter0x2F25deB3848C207fc8E0c34035B3Ba7fC157602Bas--erc20Address; the USDC token is0xcebA9300f2b948710d2653dD7B07f33A8B32118C. This is defect 1 of #2362, which this PR's description does not claim to close but whose example it edits two lines below. Failure scenario: a reader copies the first block;transfer(address,uint256)on the adapter reverts (cast calloftransfer(...)against the adapter from the example's--fromreturnsexecution reverted;decimals()on the USDC adapter also reverts, while it returns 6 on the real USDC token), so the very first command in the CLI section fails, and the USD₮ block below it, now correctly checksummed, contradicts it (token in--erc20Address, adapter in--gasCurrency). Fix shape: change line 49 to the USDC token address0xcebA9300f2b948710d2653dD7B07f33A8B32118Cand keep0x2F25deB3848C207fc8E0c34035B3Ba7fC157602Bonly on--gasCurrency(line 53); or drop #2362 from the PR's scope in words and leave the issue open with this called out. -
LOW ·
build-on-celo/fee-abstraction/using-fee-abstraction.mdx:59,62,66· The lines the PR rewrites still break two AGENTS.md rules: the intro writesUSD₮(section 7 Names: useUSDTin prose and code), and the addresses carry neither the network nor the token decimals (section 5). Failure scenario: an agent reading the block cannot tell the network or that--erc20Addressis a 6-decimal token without reading the paragraph above;--value 1000000is 1 USDT only because of that. Fix shape: write "Transfer 1 USDT" and put one line above the block, for example# Celo mainnet (42220): token 0x4806... (USDT, 6 decimals), adapter 0x0E2A... (feeCurrency), since a comment cannot sit inside the continued command. -
LOW ·
tooling/libraries-sdks/contractkit/odis.mdxneighbours and other pages · The PR fixes one legacy link and leaves the same pattern in the same directory:/developer/contractkit/...links remain oncontracts-wrappers-registry.mdx,data-encryption-key.mdx,index.mdx(3),migrating-to-contractkit-v1.mdx(2),migrating-to-contractkit-v2.mdx,migrating-to-viem.mdx,setup.mdx,usage.mdx, plus hardhat, remix, explorers, oracles and tooling overview pages. Failure scenario: none today (the wildcard redirect resolves them), but the PR text presents the odis link as the one-off cause. Fix shape: either state in the description that the other/developer/links are out of scope and file an issue, or fix the contractkit directory in the same PR (git grep -n '(/developer/' -- '*.mdx').
Verified good
- Checksums:
cast to-check-sum-addressof the old lowercase values returns exactly the new text,0x48065fbBE25f71C9282ddf5e1cD6D6A887483D5eand0x0E2A3e05bc9A16F5292A6170456A710cb89C6f72, byte for byte. Same two strings appear checksummed intooling/contracts/fee-currencies.mdxandminipay/overview.mdx. - Right asset for each flag:
0x4806...has code on Celo mainnet,symbol()isUSD₮,decimals()is 6;0x0E2A...has code (4457 hex chars),symbol()isUSD₮,decimals()is 18 (an adapter, normalised to 18), and it is inFeeCurrencyDirectory.getCurrencies(). The generatedfee-currencies.mdxrow lists feeCurrency0x0E2A...and token0x4806...for USD₮. So--erc20Addresstakes the token and--gasCurrencytakes the adapter, as the PR has it. celocli transfer:erc20(9.0.1) has exactly the flags--erc20Address,--from,--to,--value,--gasCurrency,--privateKey;--gasCurrency"must be a whitelisted token", consistent with passing the adapter. I did not run the transfer itself: it needs a funded key and sends a transaction, and the check cannot be done read-only.- DEK link:
/tooling/libraries-sdks/contractkit/data-encryption-keyis a real page (file exists, in nav), and on the rendered ODIS page every link to it (three anchors, including the sidebar) points at that path; it returns 200 with title "Data Encryption Key". No/developer/link remains onodis.mdx. - Rendering with
mint dev(merged tree): both changed pages 200, the checksummed token address present and the lowercase form absent, zero console errors, no horizontal scroll at 1280 and 412x915. - The PR description's
merge-treeclaim against #2346 and #2356 holds: pairwise merges of this branch with #2346 and #2369 are clean.
What the PR got right
Both edits are correct and mechanically verified in the description with the actual cast output. Scoping honestly: it says nothing was broken and the lowercase and redirect paths still work, rather than inventing a bug. It also knows about the neighbouring PR #2356 that touches the same USDT address.
Merge-order hazards
- #2368 (build-on-celo and tooling to build/): git carries these two edits through the renames with no conflict, but the DEK link it adds will read
/tooling/libraries-sdks/contractkit/data-encryption-keyon a branch where the page lives atbuild/tools/libraries-sdks/contractkit/data-encryption-key. It will redirect andbroken-linkswill pass, but the second merger should update the link to the new path. - #2356 edits the lowercase USDT address on
build-on-celo/build-with-ai/x402.mdx:196, a different file; no overlap. - #2346 edits other lines of
odis.mdx; the two merge cleanly in either order.
cec841c to
21e7617
Compare
|
All three addressed in 21e7617, rebased on main:
|
GigaHierz
left a comment
There was a problem hiding this comment.
head 21e7617 · API headRefOid == refs/pull/2366/head after re-fetch; 2 commits on merge-base a12466b, origin/main 7b8f97c; git merge-tree --write-tree origin/main refs/remotes/pr/2366 is CLEAN (renames followed); trial merge: mint broken-links --check-redirects "no broken links found", check-orphans.sh "No orphan pages found"; gh pr checks: Check for broken links pass; six addresses checksummed with cast to-check-sum-address; on-chain reads on forno.celo.org; celocli@9.0.1 network:whitelist --node celo run read-only
Verdict: REQUEST-CHANGES
Layout-move conflict check
No conflict. Git carries both edits through #2368's renames: build-on-celo/fee-abstraction/using-fee-abstraction.mdx becomes build/fee-abstraction/using-fee-abstraction.mdx and tooling/libraries-sdks/contractkit/odis.mdx becomes build/tools/libraries-sdks/contractkit/odis.mdx; both auto-merge. Rebase work for the author: git rebase origin/main, then repoint the one link the PR adds (finding 2) to the new path. If GitHub's own test merge reports a conflict, that is the rename blind spot; the local rebase resolves it without hand edits.
Status of earlier findings
- MEDIUM, USDC example passes the adapter as
--erc20Address: FIXED. Line 51 of the merged file now passes the token0xcebA9300f2b948710d2653dD7B07f33A8B32118C; the adapter0x2F25deB3848C207fc8E0c34035B3Ba7fC157602Bis only on--gasCurrency. Chain evidence below: read-onlyeth_calloftransfer(address,uint256)from a USDC holder succeeds on the token (true) and reverts on the adapter. - LOW, edited lines carried no network or decimals label, and prose said
USD₮: PARTLY FIXED. Both CLI blocks now open with a comment naming Celo mainnet (42220), the token with symbol and 6 decimals, and the adapter as fee currency; the CLI prose says USDT. Two defects remain: new finding 1 (the label says mainnet but the command does not select it) and new finding 3 (USD₮is still written three times in the same file). - LOW, about 14
/developer/contractkit/...links remained: REJECTED-WITH-REASON, reason accepted but the finding stays open. The author states the other/developer/links are out of scope and the wildcard redirect resolves them; that is true, and a separate concern. The count is larger than I reported:grep -rn --include='*.mdx' '(/developer/'on the merged tree finds 35 links in 15 files. Under the review rules a finding is resolved by a fix commit or by a follow-up issue that is filed and linked; no issue exists (a search of open issues finds only the epic #2266), so this is new finding 4.
New findings
- MEDIUM · build/fee-abstraction/using-fee-abstraction.mdx:48-56 and :63-71 · Each block is labelled "Celo mainnet (42220)", but the command has no
--node, andceloclidefaults tohttp://localhost:8545.
Failure scenario: a reader copies the block as labelled.celocli@9.0.1 config:getprintsnode: http://localhost:8545, and a barecelocli network:whitelistprints "Connecting to Node http://localhost:8545... !" and fails. A reader with a local node on another network would send the transfer there. The page's own allowlist block (line 20) already uses--node celo, so the guide knows the flag. AGENTS.md section 5: self-contained, network stated, "a reader copies one block and it works".
Fix shape: add--node celo \to both commands (celocli network:whitelist --node celoruns againsthttps://forno.celo.org, which proves the alias works). - LOW · build/tools/libraries-sdks/contractkit/odis.mdx:30 · The link the PR adds,
/tooling/libraries-sdks/contractkit/data-encryption-key, is a path #2368 removed; it resolves only through the/tooling/:slug*wildcard redirect.
Failure scenario: on the trial mergecurlreturns307 -> /build/tools/libraries-sdks/contractkit/data-encryption-key.mint broken-links --check-redirectsstays green because the wildcard matches, so CI will not catch it. The PR replaces one redirected legacy link with another one. The PR body says "at its real path"; after #2368 it is not.
Fix shape: change the target to/build/tools/libraries-sdks/contractkit/data-encryption-key(returns 200). - LOW · build/fee-abstraction/using-fee-abstraction.mdx:34, :88, :135 · The PR switches the CLI prose to
USDT(AGENTS.md section 7: useUSDTin prose and code, notUSD₮) but leavesUSD₮three times in the same file.
Failure scenario: one page now says USDT in one paragraph and USD₮ in the next, so a reader or agent searching the page for one spelling misses the other. AGENTS.md says to fix headings and names on pages you are already editing.
Fix shape: replaceUSD₮withUSDTon those three lines. Do not touchsymbol()output strings quoted from the chain, if any are added. - LOW · PR body and description of scope · The 35 remaining
/developer/links in 15 files (the contractkit directory, hardhat, remix, explorers, oracles, tooling overview,learn/network/overview.mdx,contribute-to-celo/release-process/smart-contracts.mdx) have no follow-up issue.
Failure scenario: "out of scope" in prose with no issue behind it evaporates, asengineering-rules.mdsection 4 warns.
Fix shape: file one issue ("replace legacy/developer/links with their final paths", with thegrepand the file list) and link it from the PR body, or fix them in a separate PR.
Verified good
- EIP-55 checksums, computed with
cast to-check-sum-addressfrom the lowercase form:0xcebA9300f2b948710d2653dD7B07f33A8B32118C,0x2F25deB3848C207fc8E0c34035B3Ba7fC157602B,0x48065fbBE25f71C9282ddf5e1cD6D6A887483D5e,0x0E2A3e05bc9A16F5292A6170456A710cb89C6f72, and also the untouched--from0x22ae7Cf4cD59773f058B685a7e6B7E0984C54966and--to0xDF7d8B197EB130cF68809730b0D41999A830c4d7. All six match the text in the PR byte for byte, and match the rows inbuild/tools/contracts/fee-currencies.mdx(lines 37 and 39). - Roles on Celo mainnet (read-only, forno.celo.org):
0xcebA...118Chas code,symbol()"USDC",decimals()6.0x2F25...602Bhas code,symbol()anddecimals()revert,adaptedToken()returns0xcebA...118C(so it is the USDC adapter, as the PR states).0x4806...3D5ehas code,symbol()"USD₮",decimals()6.0x0E2A...6f72has code,symbol()"USD₮",decimals()18,getAdaptedToken()returns0x4806...3D5e(the adapter exposesgetAdaptedToken(), notadaptedToken(), which the page already says at line 34). Both adapters are inFeeCurrencyDirectory.getCurrencies()read from0x15F344b9E6c3Cb6F0376A36A64928b13F62C6276, the address inbuild/tools/contracts/core-contracts.mdx:26; the tokens are not in the list. - Independent confirmation from the CLI:
celocli network:whitelist --node celolists USDC with whitelisted address0x2F25...602B, token address0xcebA...118C, 6 decimals, uses adapter true; and USD₮ with0x0E2A...6f72, token0x4806...3D5e, 6 decimals, uses adapter true. - The example is internally consistent: token in
--erc20Address, adapter in--gasCurrencyfor both blocks;--value 1000000is 1 USDC or 1 USDT at 6 decimals. Simulation (eth_call, no transaction):transfer(0xDF7d...c4d7, 1000000)from a USDC holder returnstrueon the token and reverts on the adapter, which is the failure the old example would have produced.celocli@9.0.1 transfer:erc20 --helplists--erc20Address,--from,--to,--value,--gasCurrency("must be a whitelisted token"),--privateKey.celocli transferitself was not run, since it sends a transaction; the PR body says the same, so the "run once with output" box in #2362 correctly stays open. - PR text matches the code: the title (USDC token, checksum USDT, DEK link) and body describe exactly the diff; the cast outputs the body quotes (adapter reverts on
decimals(),adaptedToken()returns the USDC token, adapter ingetCurrencies()) reproduce;Refs #2362is correct because the viem items and the "run once" box remain open. - Render smoke test of the trial merge (port 3523, 1280 and 412 px): both pages load, both CLI blocks show the new comment lines, token and adapter on the right flags, no lowercase USDT address on the page, zero console errors, no horizontal overflow; the ODIS page carries the DEK link at the stale
/tooling/path (finding 2). - Same-file observations that belong to the open #2362 boxes, not findings here: line 139 still passes the lowercase USDC adapter in the viem block, and
build/agents/x402.mdx:196still carries the lowercase USDT token address (in the neighbouring #2356's scope).
What the PR got right
The core defect is fixed correctly and the proof is in the body: token on --erc20Address, adapter on --gasCurrency, with the chain reads quoted. Checksums are exact. The labels carry network, symbol, decimals and the example-address caveat in one comment per block. It uses Refs rather than Closes, says what remains open in #2362, and does not claim to have run a transaction it did not run. Remaining work is one round: --node celo on two commands, one link path, three spellings, one filed issue.
Merge-order hazards and rebase work
- Rebase on origin/main (7b8f97c); no hand-resolved conflicts expected. After the rebase the touched files are
build/fee-abstraction/using-fee-abstraction.mdxandbuild/tools/libraries-sdks/contractkit/odis.mdx; the DEK link must read/build/tools/libraries-sdks/contractkit/data-encryption-key. - #2356 edits the lowercase USDT address in
build/agents/x402.mdx:196, a different file; no overlap. #2346 edits other lines ofodis.mdx; clean in either order. #2369 (CI check for redirects that cannot work) touchesdocs.jsononly.
… link the DEK page directly
…20Address The adapter reverts on transfer() and decimals(), so the first CLI example failed. Label both CLI blocks with network, token decimals and adapter, and write USDT in prose, per AGENTS.md.
…K page at its new path celocli defaults to localhost:8545, so the mainnet label alone did not pick the network. The DEK link pointed at a /tooling/ path that #2368 moved and only resolved through a redirect.
21e7617 to
7c9324d
Compare
|
Rebased onto main (clean, git followed the #2368 renames) and fixed in 7c9324d:
|
GigaHierz
left a comment
There was a problem hiding this comment.
head 7c9324d · API headRefOid == refs/pull/2366/head (re-fetched); merge base is origin/main c725175 (main moved from 7f5d9bb by #2358 and #2359 during the review, no overlap), git merge-tree --write-tree origin/main refs/remotes/pr/2366 clean; mint broken-links --check-redirects, mint validate, check-orphans.sh pass; CI run on 7c9324d green (pull_request); @celo/celocli@9.0.1 network:whitelist --node celo run read-only; cast reads on forno.celo.org; served page checked on mint dev port 3731
Verdict: APPROVE
Status of earlier findings
- MEDIUM, CLI blocks labelled "Celo mainnet (42220)" with no
--node: FIXED. Bothcelocli transfer:erc20commands now carry--node celo(using-fee-abstraction.mdx lines 51 and 67 of the head). The alias is valid:network:whitelist --node celoprints "Connecting to Node https://forno.celo.org... done" and lists USDC and USD₮ with the page's token and adapter addresses.transfer:erc20 --helplists-n, --node=<value> URL of the node to run commands against or an alias. The transfer itself was not run. - LOW, DEK link on
odis.mdxused a redirecting/tooling/...path: FIXED. It now reads/build/tools/libraries-sdks/contractkit/data-encryption-key; the file exists,mint broken-linkspasses, and the served ODIS page links that path (no/developer/or/tooling/DEK link left in the HTML). - LOW,
USD₮left three times in prose: FIXED.rg 'USD₮' build/fee-abstraction/using-fee-abstraction.mdxfinds nothing, and the rendered page has 0 occurrences.USA₮is untouched, as the issuer writes it. - LOW, 35
/developer/links with no follow-up issue: FIXED by a filed issue. #2383 is open, linked from the PR body, and its list matchesrg -n --glob '*.mdx' '\(/developer/'on the PR head: 35 links in 15 files, per-file counts and the 19 target counts all equal. (Its header says "on main at 7f5d9bb"; main still has 36 links in 16 files until this PR removes theodis.mdxone. The text two lines below explains that, so it is not a defect.)
New findings
None.
Verified good
- EIP-55: every address on both changed pages recomputed with
cast to-check-sum-address; all 8 distinct mixed-case addresses match byte for byte, including the four edited ones (0xcebA9300f2b948710d2653dD7B07f33A8B32118C,0x2F25deB3848C207fc8E0c34035B3Ba7fC157602B,0x48065fbBE25f71C9282ddf5e1cD6D6A887483D5e,0x0E2A3e05bc9A16F5292A6170456A710cb89C6f72). - Roles on Celo mainnet (read-only):
0xcebA...118Chas code,symbol()"USDC",decimals()6.0x2F25...602Bhas code,symbol()anddecimals()revert,adaptedToken()returns the USDC token.0x4806...3D5ehas code,symbol()"USD₮",decimals()6.0x0E2A...6f72has code,decimals()18,getAdaptedToken()returns0x4806...3D5e(andadaptedToken()reverts, which the page's line 34 says). Both adapters are inFeeCurrencyDirectory.getCurrencies()at0x15F344b9E6c3Cb6F0376A36A64928b13F62C6276; neither token is. So the token goes on--erc20Address, the adapter on--gasCurrency, in both blocks. - Rendered page (mint dev, port 3731, content confirmed as the PR's: 0
USD₮,--node celopresent, new label comments present): no overflow at 1280 px, zero console errors. - PR text matches the code: title, body and the quoted cast output reproduce;
Refs #2362is right because the viem items and the "run once with output" box stay open, and the body says so. gh pr checks 2366: Check for broken links pass (on 7c9324d), GitGuardian pass, StepSecurity pass; Mintlify Deployment skipped (infra).
What the PR got right
The original defect (adapter passed as the token) is fixed with chain evidence in the body, and every review round since has been fixed with a commit or a filed issue rather than prose: --node celo on both commands, one link path, three spellings, #2383 filed with the command and the file list. It does not claim to have run a transaction it did not run.
Merge-order hazards
- None. Touches
build/fee-abstraction/using-fee-abstraction.mdxandbuild/tools/libraries-sdks/contractkit/odis.mdxonly; #2358 and #2359 (already on main) do not overlap. #2369 and #2382 touchdocs.jsonand scripts only. - Same-file items that remain in #2362's open boxes, not findings here: the viem block at line 141 still passes the USDC adapter in lowercase (
0x2f25...602b; valid for viem, covered by the "label every address" box), andbuild/agents/x402.mdx:196is in #2356's scope.
The first
celocli transfer:erc20example in the fee abstraction guide passed the USDC adapter (0x2F25deB3848C207fc8E0c34035B3Ba7fC157602B) as--erc20Address, so it failed: the adapter reverts ontransfer()anddecimals(). This passes the USDC token (0xcebA9300f2b948710d2653dD7B07f33A8B32118C) instead and keeps the adapter on--gasCurrency, the same way the USDT example below it already worked. It also checksums the USDT token and adapter addresses in that example to matchbuild/tools/contracts/fee-currencies.mdx, and links the ODIS page to the data encryption key page at its real path,/build/tools/libraries-sdks/contractkit/data-encryption-key, instead of the legacy/developer/contractkit/...path.Refs #2362. This fixes defect 1 of that issue (adapter vs token in the CLI example) and the address labels in the CLI blocks. The viem decimals, imports and labels are still open, so the issue stays open. Both CLI blocks now open with a comment that gives the network (Celo mainnet, 42220), the token address and its decimals, the adapter used as the fee currency, and that
--fromand--toare example addresses, as AGENTS.md sections 5 and 7 ask. Both commands pass--node celo, becausecelocliotherwise connects tohttp://localhost:8545. The page writes USDT everywhere in prose instead of USD₮.Other pages still link through
/developer/...paths (35 links in 15 files, including the rest of the contractkit directory). The wildcard redirects resolve them; #2383 tracks replacing them with their final paths.I did not run the transfers, because they need a funded key and send real transactions on mainnet, so #2362's "run once with output" box is still open. I checked the addresses read-only on forno.celo.org: