2025 07 15 supply 20#362
Conversation
WalkthroughThis change re-enables and updates the ERC20 total supply opcodes in the interpreter's standard operations library. It modernizes their implementation to use floating-point representations, updates their function signatures and stack handling, and fully restores and adapts their test suites. Gas snapshot metrics are also refreshed to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Interpreter
participant LibAllStandardOps
participant LibOpERC20TotalSupply
participant ERC20Token
Interpreter->>LibAllStandardOps: Calls opcodeFunctionPointers() for ERC20 total supply
LibAllStandardOps->>LibOpERC20TotalSupply: run(state, operand, stackTop)
LibOpERC20TotalSupply->>ERC20Token: totalSupply()
LibOpERC20TotalSupply->>ERC20Token: decimals()
LibOpERC20TotalSupply->>LibOpERC20TotalSupply: Convert totalSupply to Float
LibOpERC20TotalSupply-->>Interpreter: Return result on stack
Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
src/generated/Rainterpreter.pointers.solis excluded by!**/generated/**src/generated/RainterpreterExpressionDeployer.pointers.solis excluded by!**/generated/**src/generated/RainterpreterParser.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (7)
.gas-snapshot(8 hunks)lib/rain.interpreter.interface(1 hunks)src/lib/op/LibAllStandardOps.sol(7 hunks)src/lib/op/erc20/LibOpERC20TotalSupply.sol(1 hunks)src/lib/op/erc20/uint256/LibOpUint256ERC20TotalSupply.sol(2 hunks)test/src/lib/op/erc20/LibOpERC20TotalSupply.t.sol(1 hunks)test/src/lib/op/erc20/uint256/LibOpUint256ERC20TotalSupply.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:0-0
Timestamp: 2025-07-15T11:31:27.986Z
Learning: In the rainlanguage/rain.interpreter project, forge (Foundry's formatting tool) handles code formatting automatically, so formatting-related suggestions are not actionable.
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:35-36
Timestamp: 2025-07-15T11:31:10.064Z
Learning: In the rain.interpreter codebase, when working with ERC20 tokens that may not implement the optional decimals() function, the preference is to let the call fail explicitly rather than catching errors and defaulting to 18 decimals. This ensures correctness by avoiding potentially incorrect calculations with assumed decimal values.
lib/rain.interpreter.interface (2)
undefined
<retrieved_learning>
Learnt from: thedavidmeister
PR: #360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:0-0
Timestamp: 2025-07-15T11:31:27.986Z
Learning: In the rainlanguage/rain.interpreter project, forge (Foundry's formatting tool) handles code formatting automatically, so formatting-related suggestions are not actionable.
</retrieved_learning>
<retrieved_learning>
Learnt from: thedavidmeister
PR: #360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:0-0
Timestamp: 2025-07-15T11:31:35.609Z
Learning: In the rain.interpreter codebase, the team uses forge for automatic code formatting, so manual formatting suggestions are not needed as the tool will handle formatting automatically.
</retrieved_learning>
src/lib/op/erc20/LibOpERC20TotalSupply.sol (1)
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:35-36
Timestamp: 2025-07-15T11:31:10.064Z
Learning: In the rain.interpreter codebase, when working with ERC20 tokens that may not implement the optional decimals() function, the preference is to let the call fail explicitly rather than catching errors and defaulting to 18 decimals. This ensures correctness by avoiding potentially incorrect calculations with assumed decimal values.
test/src/lib/op/erc20/LibOpERC20TotalSupply.t.sol (1)
Learnt from: thedavidmeister
PR: rainlanguage/rain.interpreter#360
File: src/lib/op/erc20/LibOpERC20Allowance.sol:35-36
Timestamp: 2025-07-15T11:31:10.064Z
Learning: In the rain.interpreter codebase, when working with ERC20 tokens that may not implement the optional decimals() function, the preference is to let the call fail explicitly rather than catching errors and defaulting to 18 decimals. This ensures correctness by avoiding potentially incorrect calculations with assumed decimal values.
⏰ 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). (10)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, test-wasm-build)
- GitHub Check: rainix (macos-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
- GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
- GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
- GitHub Check: git-clean
🔇 Additional comments (14)
lib/rain.interpreter.interface (1)
1-1: Submodule bump requires downstream verificationOnly the commit pointer changed. Please double-check that:
- Every consumer repo is updated (
forge test/CI green) against70d9fae4….- The commit is on the canonical
rain.interpreter.interfacemain branch or a signed tag to avoid supply-chain surprises.If both hold, nothing further to do.
src/lib/op/erc20/uint256/LibOpUint256ERC20TotalSupply.sol (1)
32-42: LGTM! Proper modernization to use StackItem types.The function signature and implementation correctly handle the conversion between StackItem and native types, aligning with the interpreter's stack abstraction.
src/lib/op/LibAllStandardOps.sol (4)
37-37: Correct updates for re-enabling ERC20 total supply opcodes.The import is properly uncommented and the ops count is correctly incremented by 2.
Also applies to: 108-108
171-174: Metadata entries correctly describe the opcodes.Both ERC20 total supply opcodes have clear descriptions differentiating the uint256 (raw) version from the floating-point version.
Also applies to: 183-185
392-393: Operand handlers correctly configured.Both opcodes appropriately use
handleOperandDisallowedas they don't accept operands.Also applies to: 398-399
559-559: Function pointers correctly integrated.Integrity and run function pointers are properly added at consistent positions across arrays.
Also applies to: 562-562, 671-671, 674-674
src/lib/op/erc20/LibOpERC20TotalSupply.sol (2)
16-20: Integrity function correctly specifies I/O.The function properly indicates 1 input (token address) and 1 output (total supply).
22-38: Run function properly implements floating-point conversion.The implementation correctly handles total supply retrieval and decimal conversion, with appropriate documentation that
decimals()may fail as it's optional in the ERC20 standard.test/src/lib/op/erc20/uint256/LibOpUint256ERC20TotalSupply.t.sol (1)
1-74: Test suite properly modernized with comprehensive coverage.The tests correctly use
OperandV2andStackItemtypes, with proper wrapping/unwrapping of values and good coverage of various scenarios including error cases.test/src/lib/op/erc20/LibOpERC20TotalSupply.t.sol (5)
18-23: LGTM!The integrity check test correctly verifies that the opcode requires 1 input and produces 1 output.
55-65: LGTM!The happy path test correctly validates the evaluation of total supply with proper mocking and lossless conversion checks.
68-81: LGTM!Excellent overflow test coverage. The test properly ensures lossy conversion at both the decimal conversion and packing stages before validating the expected error.
84-98: LGTM!The input/output validation tests comprehensively cover edge cases for incorrect argument counts.
101-103: LGTM!The test correctly validates that operands are disallowed for this opcode.
| function referenceFn(InterpreterState memory, OperandV2, StackItem[] memory inputs) | ||
| internal | ||
| view | ||
| returns (StackItem[] memory) | ||
| { | ||
| address account = address(uint160(uint256(StackItem.unwrap(inputs[0])))); | ||
| uint256 totalSupply = IERC20(account).totalSupply(); | ||
|
|
||
| uint8 tokenDecimals = IERC20Metadata(address(uint160(account))).decimals(); | ||
| Float totalSupplyFloat = LibDecimalFloat.fromFixedDecimalLosslessPacked(totalSupply, tokenDecimals); | ||
|
|
||
| StackItem[] memory outputs = new StackItem[](1); | ||
| outputs[0] = StackItem.wrap(Float.unwrap(totalSupplyFloat)); | ||
| return outputs; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove redundant address casting.
The variable account is already an address type, so the casting on line 48 is unnecessary.
- uint8 tokenDecimals = IERC20Metadata(address(uint160(account))).decimals();
+ uint8 tokenDecimals = IERC20Metadata(account).decimals();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function referenceFn(InterpreterState memory, OperandV2, StackItem[] memory inputs) | |
| internal | |
| view | |
| returns (StackItem[] memory) | |
| { | |
| address account = address(uint160(uint256(StackItem.unwrap(inputs[0])))); | |
| uint256 totalSupply = IERC20(account).totalSupply(); | |
| uint8 tokenDecimals = IERC20Metadata(address(uint160(account))).decimals(); | |
| Float totalSupplyFloat = LibDecimalFloat.fromFixedDecimalLosslessPacked(totalSupply, tokenDecimals); | |
| StackItem[] memory outputs = new StackItem[](1); | |
| outputs[0] = StackItem.wrap(Float.unwrap(totalSupplyFloat)); | |
| return outputs; | |
| } | |
| function referenceFn(InterpreterState memory, OperandV2, StackItem[] memory inputs) | |
| internal | |
| view | |
| returns (StackItem[] memory) | |
| { | |
| address account = address(uint160(uint256(StackItem.unwrap(inputs[0])))); | |
| uint256 totalSupply = IERC20(account).totalSupply(); | |
| uint8 tokenDecimals = IERC20Metadata(account).decimals(); | |
| Float totalSupplyFloat = LibDecimalFloat.fromFixedDecimalLosslessPacked(totalSupply, tokenDecimals); | |
| StackItem[] memory outputs = new StackItem[](1); | |
| outputs[0] = StackItem.wrap(Float.unwrap(totalSupplyFloat)); | |
| return outputs; | |
| } |
🤖 Prompt for AI Agents
In src/lib/op/erc20/LibOpERC20TotalSupply.sol between lines 40 and 54, the
variable `account` is assigned by casting the input to an address with redundant
nested casts. Simplify this by removing the unnecessary inner cast to uint160
and directly cast the unwrapped input to address. This will clean up the code
without changing functionality.
| external | ||
| { | ||
| assumeEtchable(account); | ||
| vm.etch(account, hex"fe"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using more realistic contract bytecode.
While hex"fe" (invalid opcode) works for making an address etchable, consider using more realistic ERC20 contract bytecode or at least a valid minimal contract bytecode for better test fidelity.
🤖 Prompt for AI Agents
In test/src/lib/op/erc20/LibOpERC20TotalSupply.t.sol at line 29, replace the
current minimal bytecode hex"fe" with a more realistic or valid minimal contract
bytecode to improve test fidelity. Use actual ERC20 contract bytecode or a valid
minimal contract snippet instead of an invalid opcode to better simulate real
contract behavior during testing.
| function testOpERC20TotalSupplyNPRun(address account, uint256 totalSupply, uint16 operandData, uint8 decimals) | ||
| external | ||
| { | ||
| assumeEtchable(account); | ||
| vm.etch(account, hex"fe"); | ||
|
|
||
| (, bool lossless) = LibDecimalFloat.fromFixedDecimalLossyPacked(totalSupply, decimals); | ||
| vm.assume(lossless); | ||
|
|
||
| StackItem[] memory inputs = new StackItem[](1); | ||
| inputs[0] = StackItem.wrap(bytes32(uint256(uint160(account)))); | ||
| OperandV2 operand = LibOperand.build(1, 1, operandData); | ||
|
|
||
| vm.mockCall(account, abi.encodeWithSelector(IERC20.totalSupply.selector), abi.encode(totalSupply)); | ||
| // called once for reference, once for run | ||
| vm.expectCall(account, abi.encodeWithSelector(IERC20.totalSupply.selector), 2); | ||
|
|
||
| vm.mockCall(account, abi.encodeWithSelector(IERC20Metadata.decimals.selector), abi.encode(decimals)); | ||
|
|
||
| opReferenceCheck( | ||
| opTestDefaultInterpreterState(), | ||
| operand, | ||
| LibOpERC20TotalSupply.referenceFn, | ||
| LibOpERC20TotalSupply.integrity, | ||
| LibOpERC20TotalSupply.run, | ||
| inputs | ||
| ); | ||
| } | ||
|
|
||
| /// Test the eval of totalSupply parsed from a string. | ||
| function testOpERC20TotalSupplyEvalHappy(uint256 totalSupply, uint8 decimals) external { | ||
| (, bool lossless) = LibDecimalFloat.fromFixedDecimalLossyPacked(totalSupply, decimals); | ||
| vm.assume(lossless); | ||
|
|
||
| vm.mockCall(address(0xdeadbeef), abi.encodeWithSelector(IERC20.totalSupply.selector), abi.encode(totalSupply)); | ||
| vm.mockCall(address(0xdeadbeef), abi.encodeWithSelector(IERC20Metadata.decimals.selector), abi.encode(decimals)); | ||
|
|
||
| Float totalSupplyFloat = LibDecimalFloat.fromFixedDecimalLosslessPacked(totalSupply, decimals); | ||
|
|
||
| checkHappy("_: erc20-total-supply(0xdeadbeef);", Float.unwrap(totalSupplyFloat), "0xdeadbeef 0xdeadc0de"); | ||
| } | ||
|
|
||
| /// Test overflow of totalSupply. | ||
| function testOpERC20TotalSupplyEvalOverflow(uint256 totalSupply, uint8 decimals) external { | ||
| (int256 signedCoefficient, int256 exponent, bool lossless) = | ||
| LibDecimalFloat.fromFixedDecimalLossy(totalSupply, decimals); | ||
| vm.assume(!lossless); | ||
| (, bool losslessPack) = LibDecimalFloat.packLossy(signedCoefficient, exponent); | ||
| vm.assume(!losslessPack); | ||
|
|
||
| vm.mockCall(address(0xdeadbeef), abi.encodeWithSelector(IERC20.totalSupply.selector), abi.encode(totalSupply)); | ||
| vm.mockCall(address(0xdeadbeef), abi.encodeWithSelector(IERC20Metadata.decimals.selector), abi.encode(decimals)); | ||
| checkUnhappy( | ||
| "_: erc20-total-supply(0xdeadbeef);", | ||
| abi.encodeWithSelector(LossyConversionToFloat.selector, signedCoefficient, exponent) | ||
| ); | ||
| } | ||
|
|
||
| /// Test that a totalSupply with bad inputs fails integrity. | ||
| function testOpERC20TotalSupplyEvalZeroInputs() external { | ||
| checkBadInputs("_: erc20-total-supply();", 0, 1, 0); | ||
| } | ||
|
|
||
| function testOpERC20TotalSupplyEvalTwoInputs() external { | ||
| checkBadInputs("_: erc20-total-supply(0xdeadbeef 0xdeadc0de);", 2, 1, 2); | ||
| } | ||
|
|
||
| function testOpERC20TotalSupplyEvalZeroOutputs() external { | ||
| checkBadOutputs(": erc20-total-supply(0xdeadbeef);", 1, 1, 0); | ||
| } | ||
|
|
||
| function testOpERC20TotalSupplyEvalTwoOutputs() external { | ||
| checkBadOutputs("_ _: erc20-total-supply(0xdeadbeef);", 1, 1, 2); | ||
| } | ||
|
|
||
| /// Test that operand is disallowed. | ||
| function testOpERC20TotalSupplyEvalOperandDisallowed() external { | ||
| checkUnhappyParse("_: erc20-total-supply<0>(0xdeadbeef);", abi.encodeWithSelector(UnexpectedOperand.selector)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding edge case tests.
While the test suite is comprehensive, consider adding tests for:
- A test documenting the behavior when
decimals()call fails (even though letting it fail is intentional per the codebase convention) - Edge case where
totalSupplyreturns 0 - A test with maximum uint256 total supply value
These additions would further strengthen the test coverage.
Would you like me to generate these additional test cases?
🤖 Prompt for AI Agents
In test/src/lib/op/erc20/LibOpERC20TotalSupply.t.sol between lines 25 and 103,
the current tests lack coverage for some edge cases. Add tests for the scenario
where the decimals() call reverts or fails, a case where totalSupply returns
zero, and a test using the maximum uint256 value for totalSupply. These tests
should mock the respective calls to simulate failure or edge values and verify
the expected behavior or error handling in each case.
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Refactor
Tests
Chores