2025 07 15 allowance op - #360
Conversation
WalkthroughThe changes activate and integrate the Changes
Sequence Diagram(s)sequenceDiagram
participant Interpreter as Interpreter
participant LibAllStandardOps as LibAllStandardOps
participant LibOpERC20Allowance as LibOpERC20Allowance
participant ERC20 as ERC20 (IERC20Metadata)
Interpreter->>LibAllStandardOps: Execute opcode "erc20-allowance"
LibAllStandardOps->>LibOpERC20Allowance: run(state, operandV2, stackTop)
LibOpERC20Allowance->>ERC20: allowance(owner, spender)
ERC20-->>LibOpERC20Allowance: allowanceValue (uint256)
LibOpERC20Allowance->>ERC20: decimals()
ERC20-->>LibOpERC20Allowance: decimals (uint8)
LibOpERC20Allowance->>LibOpERC20Allowance: Convert allowanceValue to Float using decimals
LibOpERC20Allowance-->>LibAllStandardOps: Return Float result
LibAllStandardOps-->>Interpreter: Result on stack
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/lib/op/erc20/LibOpERC20Allowance.sol (6)⏰ 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). (7)
🔇 Additional comments (3)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
test/src/lib/op/erc20/LibOpERC20Allowance.t.sol (1)
24-98: Remove dead commented-out tests in LibOpERC20Allowance.t.solThere’s a large block of fully commented test functions (e.g.
testOpERC20AllowanceNPRun,testOpERC20AllowanceNPEvalHappy, and the subsequenttestOpERC20AllowanceNPEval*cases) between approximately lines 24–98. These stale comments are triggering formatting failures.• File: test/src/lib/op/erc20/LibOpERC20Allowance.t.sol
– Remove (or, if still needed, uncomment and reformat) the entire commented-out functions block.
– After cleanup, run your formatter (e.g.forge fmt) to verify no lint/format errors remain.
– Commit the deletion of all lines beginning with// function testOpERC20Allowance…and accompanying commented code.
📜 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 (3)
src/lib/op/LibAllStandardOps.sol(6 hunks)src/lib/op/erc20/LibOpERC20Allowance.sol(1 hunks)test/src/lib/op/erc20/LibOpERC20Allowance.t.sol(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Git is clean
test/src/lib/op/erc20/LibOpERC20Allowance.t.sol
[error] 21-98: Git diff detected changes in the file indicating uncommitted or unexpected modifications (commented/uncommented code changes).
src/lib/op/erc20/LibOpERC20Allowance.sol
[error] 35-55: Git diff detected changes in the file indicating uncommitted or unexpected modifications.
🪛 GitHub Actions: Rainix CI
test/src/lib/op/erc20/LibOpERC20Allowance.t.sol
[error] 24-97: Foundry fmt check failed: commented-out code lines differ in formatting style. Consider consistent comment formatting.
src/lib/op/erc20/LibOpERC20Allowance.sol
[error] 38-41: Foundry fmt check failed: multiline formatting differs. Suggested fix: combine multiple lines into a single line for 'Float tokenAllowanceFloat = LibDecimalFloat.fromFixedDecimalLosslessPacked(...)'.
[error] 58-64: Foundry fmt check failed: multiline formatting differs. Suggested fix: combine multiple lines into a single line for 'Float tokenAllowanceFloat = LibDecimalFloat.fromFixedDecimalLosslessPacked(...)'.
🔇 Additional comments (12)
test/src/lib/op/erc20/LibOpERC20Allowance.t.sol (2)
4-7: LGTM - Import statements activated correctly.The import statements have been properly uncommented and are consistent with the updated library implementation.
14-23: Test contract activated with correct signature updates.The test contract has been properly activated with the function signature updated to use
OperandV2. The test correctly verifies the opcode expects 3 inputs and 1 output.src/lib/op/LibAllStandardOps.sol (6)
35-35: Import statement properly activated.The import for
LibOpERC20Allowancehas been correctly uncommented and positioned appropriately in the import section.
108-108: Opcode count updated correctly.The
ALL_STANDARD_OPS_LENGTHhas been incremented to 35 to account for the newly activatederc20-allowanceopcode.
175-178: Opcode metadata properly defined.The authoring metadata for the
erc20-allowanceopcode is comprehensive and includes important information about input parameters and saturation behavior on overflow.
394-395: Operand handler correctly configured.The operand handler is properly set to
handleOperandDisallowed, which is appropriate for this opcode that doesn't accept operands.
560-560: Integrity function pointer properly integrated.The integrity function pointer for
LibOpERC20Allowance.integrityhas been correctly added to the function pointer array.
672-672: Runtime function pointer properly integrated.The runtime function pointer for
LibOpERC20Allowance.runhas been correctly added to the opcode function pointer array.src/lib/op/erc20/LibOpERC20Allowance.sol (4)
4-11: Import statements updated correctly.The import statements have been properly uncommented and updated to include the new dependencies for floating-point operations (
LibDecimalFloat,Float) and stack operations (StackItem).
16-20: Function signature updated correctly.The integrity function signature has been properly updated to use
OperandV2instead ofOperand, maintaining the correct input/output counts (3 inputs, 1 output).
22-47: Enhanced implementation with floating-point conversion.The
runfunction has been significantly improved to convert raw allowance values to floating-point representation using the token's decimal precision. This provides more consistent handling of allowance values across different tokens.
49-69: Reference function updated with new type system.The reference function has been properly updated to use
StackItemarrays and floating-point conversion, maintaining consistency with the runtime implementation.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/lib/op/erc20/LibOpERC20Allowance.sol (1)
35-36: Handle optional decimals() call gracefully.The
decimals()call can fail since it's optional in the ERC20 standard. This issue was previously identified and should be addressed with proper error handling.- // This can fail as `decimals` is an OPTIONAL part of the ERC20 standard. - uint8 tokenDecimals = IERC20Metadata(address(uint160(token))).decimals(); + // This can fail as `decimals` is an OPTIONAL part of the ERC20 standard. + uint8 tokenDecimals; + try IERC20Metadata(address(uint160(token))).decimals() returns (uint8 decimals) { + tokenDecimals = decimals; + } catch { + tokenDecimals = 18; // Default to 18 decimals if not available + }
📜 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 (6)
.gas-snapshot(18 hunks)src/lib/op/LibAllStandardOps.sol(7 hunks)src/lib/op/erc20/LibOpERC20Allowance.sol(1 hunks)src/lib/op/erc20/uint256/LibOpUint256ERC20Allowance.sol(2 hunks)test/src/lib/op/erc20/LibOpERC20Allowance.t.sol(1 hunks)test/src/lib/op/erc20/uint256/LibOpUint256ERC20Allowance.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/lib/op/erc20/LibOpERC20Allowance.sol (1)
Learnt from: 0xgleb
PR: rainlanguage/rain.interpreter#334
File: crates/eval/src/fork.rs:489-494
Timestamp: 2025-06-17T10:56:40.904Z
Learning: When analyzing error handling in Rust codebases that use external crates like foundry-evm, verify actual compilation behavior rather than assuming missing trait implementations. The `?` operator often works due to comprehensive error conversion implementations provided by the crate ecosystem.
🔇 Additional comments (15)
src/lib/op/erc20/uint256/LibOpUint256ERC20Allowance.sol (1)
38-50: LGTM! Type safety improvements are well implemented.The update to use
StackItem[]for inputs and outputs enhances type safety while maintaining the correct logic flow. The function properly unwraps inputs, performs the allowance query, and wraps the result appropriately..gas-snapshot (1)
131-140: Expected gas snapshot updates from opcode activation.The addition of new gas metrics for
LibOpERC20AllowanceTestandLibOpUint256ERC20AllowanceTestaligns with the activation of ERC20 allowance opcodes in the standard operations set.src/lib/op/LibAllStandardOps.sol (6)
35-35: LGTM! Proper opcode activation.The import of
LibOpERC20Allowancecorrectly activates the previously commented-out opcode.
108-108: Opcode count correctly updated.The
ALL_STANDARD_OPS_LENGTHis properly incremented from 34 to 36, reflecting the activation of bothuint256-erc20-allowanceanderc20-allowanceopcodes.
163-178: Well-documented opcode metadata.The authoring metadata provides clear descriptions for both allowance opcodes, correctly noting the lossy float conversion for the
erc20-allowancevariant to handle "infinite approve" scenarios.
388-395: Operand handlers correctly configured.Both allowance opcodes are properly configured to disallow operands, which is appropriate for these parameter-less operations.
557-560: Integrity function pointers properly added.The integrity function pointers for both allowance opcodes are correctly integrated into the standard operations array.
669-672: Runtime function pointers properly added.The runtime function pointers for both allowance opcodes are correctly integrated into the standard operations array, completing the opcode activation.
test/src/lib/op/erc20/uint256/LibOpUint256ERC20Allowance.t.sol (4)
16-21: LGTM: Integrity test correctly validates input/output countsThe integrity test properly validates that the ERC20 allowance opcode requires exactly 3 inputs (token, owner, spender) and produces 1 output (allowance value).
23-45: LGTM: Runtime test provides comprehensive coverageThe runtime test properly:
- Uses
assumeEtchableto ensure the token address is valid for mocking- Wraps inputs as
StackItemvalues following the updated interface- Mocks the ERC20 allowance call with appropriate parameters
- Verifies the call is made exactly twice (once for reference, once for run)
- Uses
opReferenceCheckto validate consistency between reference and actual implementation
47-59: LGTM: String parsing test validates end-to-end functionalityThe test properly validates that the opcode can be parsed from a string expression and produces the expected output when the ERC20 allowance call is mocked.
61-92: LGTM: Comprehensive error condition testingThe test suite properly covers all error conditions:
- Invalid input counts (0, 1, 2, 4 inputs when 3 are required)
- Invalid output counts (0, 2 outputs when 1 is required)
- Operand disallowance validation
test/src/lib/op/erc20/LibOpERC20Allowance.t.sol (3)
18-23: LGTM: Integrity test correctly validates input/output countsThe integrity test properly validates that the ERC20 allowance opcode requires exactly 3 inputs and produces 1 output, consistent with the uint256 variant.
54-68: LGTM: Float conversion test logic is soundThe test properly:
- Mocks both the allowance call and the decimals call
- Converts the allowance to floating-point representation using the token's decimals
- Validates the result matches the expected float value
The float conversion approach is appropriate for this opcode variant.
70-101: LGTM: Comprehensive error condition testingThe test suite properly covers all error conditions with appropriate opcode name (
erc20-allowanceinstead ofuint256-erc20-allowance):
- Invalid input counts (0, 1, 2, 4 inputs when 3 are required)
- Invalid output counts (0, 2 outputs when 1 is required)
- Operand disallowance validation
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes