Add guidelines for numeric literals #5936
Conversation
|
Amxx
force-pushed
the
guidelines/hex-dec
branch
from
d1e43b3 to
351963b
Compare
WalkthroughThis PR standardizes numeric literals in Solidity and inline assembly across multiple files to use hexadecimal for memory locations, offsets, and specific lengths, and decimal for bit shift counts. It updates GUIDELINES.md with formatting rules and reorganizes bullets. In contracts, it reformats constants in ERC7579 utils, clones with immutable args, SafeERC20, Bytes, Create2, LowLevelCall, RelayedCall, ShortStrings, SignatureChecker, and Checkpoints (including the JS template). Changes include 0→0x00 and 32→0x20 style edits, with equivalent bounds and logic. No exported/public signatures change, and behavior remains the same. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
contracts/utils/structs/Checkpoints.sol (3)
423-425: Same correctness for Trace224 accessor0x00/0x20 usage looks good.
626-628: Same correctness for Trace208 accessorNo functional change; aligned with style.
829-831: Same correctness for Trace160 accessorNo functional change; aligned with style.
🧹 Nitpick comments (3)
contracts/account/utils/draft-ERC7579Utils.sol (1)
166-168: Consistent zero formattingMinor consistency nit: use 0x00 (hex) for the start index to match the rest of the file/guidelines.
- target = address(bytes20(executionCalldata[0:0x14])); + target = address(bytes20(executionCalldata[0x00:0x14]));GUIDELINES.md (1)
157-169: Fix typo, list style, and use keccak256 in GUIDELINES.md (lines 157–169)Typo "arithmetics" still present at line 157; apply the diff below.
-* In some cases arithmetics value should be written in specific formats +- In some cases, arithmetic values should be written in specific formats - - * Memory locations should be written in hex format: `mload(64)` → `mload(0x40)` - - * Memory offset should be written in hex format: `mstore(add(ptr, 32), value)` → `mstore(add(ptr, 0x20), value)` - - * [Exception] for trivially small values, such as 1 or 2, decimal format can be accepted `ptr := add(ptr, 1)` - - * Memory length should be written in hex format: `keccak(ptr, 85)` → `keccak(ptr, 0x55)` - - * [Exception] in call/staticcall/delegatecall, decimal zero can be used if both the location and the length are zero. - - * Bits offset used in shifts should be written in decimal format: `shl(0x80, value)` → `shl(128, value)` + - Memory locations should be written in hex format: `mload(64)` → `mload(0x40)` + - Memory offsets should be written in hex format: `mstore(add(ptr, 32), value)` → `mstore(add(ptr, 0x20), value)` + - [Exception] For trivially small values (e.g., 1 or 2), decimal format can be accepted: `ptr := add(ptr, 1)` + - Memory length should be written in hex format: `keccak256(ptr, 85)` → `keccak256(ptr, 0x55)` + - [Exception] In call/staticcall/delegatecall, decimal zero may be used if both the location and the length are zero. + - Bit offsets used in shifts should be written in decimal format: `shl(0x80, value)` → `shl(128, value)`contracts/proxy/Clones.sol (1)
283-288: _cloneCodeWithImmutableArgs: bound/length constants correct; normalize hex shift counts (nit)
- 0x5fd3 == 24531; args.length + 0x2d encodes the PUSH2 total length — EIP‑170 respected.
- Nit: prefer decimal bit offsets for shl/shr. Occurrences in contracts/proxy/Clones.sol — lines 54, 56, 101, 103: shr(0xe8) → shr(232), shl(0x60) → shl(96), shl(0x78) → shl(120). Convert in a follow‑up for consistency/readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
GUIDELINES.md(3 hunks)contracts/account/utils/draft-ERC7579Utils.sol(5 hunks)contracts/proxy/Clones.sol(2 hunks)contracts/token/ERC20/utils/SafeERC20.sol(3 hunks)contracts/utils/Bytes.sol(1 hunks)contracts/utils/Create2.sol(1 hunks)contracts/utils/LowLevelCall.sol(5 hunks)contracts/utils/RelayedCall.sol(2 hunks)contracts/utils/ShortStrings.sol(4 hunks)contracts/utils/cryptography/SignatureChecker.sol(1 hunks)contracts/utils/structs/Checkpoints.sol(4 hunks)scripts/generate/templates/Checkpoints.js(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codespell
GUIDELINES.md
[failure] 157-157:
arithmetics ==> arithmetic
🪛 markdownlint-cli2 (0.17.2)
GUIDELINES.md
157-157: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
159-159: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
161-161: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
163-163: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
165-165: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
167-167: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
169-169: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: tests
- GitHub Check: tests-foundry
🔇 Additional comments (29)
scripts/generate/templates/Checkpoints.js (1)
225-226: Hex scratch-space usage is consistent and correct.Switching to mstore(0x00, …) and keccak256(0x00, 0x20) matches the new guideline and preserves behavior. LGTM.
contracts/utils/Bytes.sol (1)
226-226: 0x20 stride matches 32‑byte chunking.The i += 0x20 step is equivalent to 32 and aligns with word reads. LGTM.
contracts/utils/LowLevelCall.sol (5)
66-69: Use of 0x00 as out offset for staticcall is correct.Return values still loaded from 0x00/0x20. No semantic change. LGTM.
75-76: delegatecallNoReturn: hex zeroes.Out offset/size set to 0x00 is equivalent to 0. LGTM.
89-92: delegatecallReturn64Bytes: hex scratch space.Reading results from 0x00/0x20 remains correct. LGTM.
105-109: returnData: returndatacopy source 0x00.Consistent with scratch-space convention. LGTM.
115-118: bubbleRevert: copy from 0x00.No behavior change; matches guideline. LGTM.
contracts/token/ERC20/utils/SafeERC20.sol (6)
179-186: _safeTransfer: hex in call args.Using 0x00/0x44/0x20 is stylistic; behavior unchanged. LGTM.
189-192: _safeTransfer: revert bubble uses 0x00.returndatacopy from 0x00 matches convention. LGTM.
221-228: _safeTransferFrom: hex in call args.No semantic change; consistent with rest. LGTM.
232-235: _safeTransferFrom: revert bubble uses 0x00.Consistent, no behavior change. LGTM.
258-265: _safeApprove: hex in call args.Semantics preserved. LGTM.
268-271: _safeApprove: revert bubble uses 0x00.Consistent with guideline. LGTM.
contracts/utils/cryptography/SignatureChecker.sol (1)
85-86: ERC1271 staticcall output offset set to 0x00.Load/check from 0x00 remains unchanged; logic intact. LGTM.
contracts/utils/Create2.sol (2)
73-82: Comment diagram updated to hex length.The memory layout comment now reflects 0x55 bytes; clearer and consistent. LGTM.
88-88: Hash length 0x55 (85) is correct for 0xff||deployer||salt||bytecodeHash.No behavior change. LGTM.
contracts/utils/RelayedCall.sol (2)
111-111: Initcode prefix store uses 0x00 offset.Purely cosmetic; fine. LGTM.
123-124: Bubble revert: copy from 0x00.Matches new guideline; behavior unchanged. LGTM.
contracts/utils/ShortStrings.sol (4)
55-56: Threshold 0x1f for short strings.Hex literal clarifies 31‑byte limit. LGTM.
67-70: Allocate string(0x20).Hex 32 improves consistency with word size. LGTM.
80-81: byteLength guard uses 0x1f.Consistent with 31‑byte cap. LGTM.
89-94: Fallback threshold uses 0x20.Hex literal for 32‑byte boundary; behavior unchanged. LGTM.
contracts/proxy/Clones.sol (1)
262-265: fetchCloneArgs: hex offsets/lengths LGTM
- new bytes(instance.code.length - 0x2d) safely reverts on underflow (too short).
- extcodecopy uses add(result, 0x20) for data pointer; unchanged semantics.
contracts/account/utils/draft-ERC7579Utils.sol (3)
129-133: decodeMode: hex offsets for field extraction are correctPure formatting; behavior unchanged.
149-152: decodeSingle: hex slice boundaries are correct (0x00..0x34)Matches ABI layout: 20 + 32 then tail.
183-209: decodeBatch: hex literals in bounds checks/read are correct0x20 conversions preserve semantics; overflow/underflow guards remain intact.
GUIDELINES.md (2)
120-120: Whitespace-only tweakNo issues.
143-156: Custom errors bullets reflowOrdering/readability improved; content intact.
contracts/utils/structs/Checkpoints.sol (1)
220-222: Hex memory addressing is correct and generator template mirrors changeUsing 0x00/0x20 in mstore/keccak256 is semantically identical; scripts/generate/templates/Checkpoints.js implements the same pattern with dynamic checkpointSize (pos or mul(pos, N)), matching contracts/utils/structs/Checkpoints.sol.
|
|
||
| assembly ("memory-safe") { | ||
| success := call(reqGas, to, value, add(data, 0x20), mload(data), 0, 0) | ||
| success := call(reqGas, to, value, add(data, 0x20), mload(data), 0x00, 0x00) |
There was a problem hiding this comment.
According to the guidelines this are acceptable as decimal zero. Not a big deal.
ernestognw
changed the title
2 participants
No description provided.