2025 10 04 float #6
Conversation
WalkthroughMigrate interpreter integration to OperandV2/StackItem and IInterpreterExternV4; change numeric types to Float across LibOpPythPrice and LibPyth; update tests to use Float/StackItem; adjust foundry remappings and optimizer runs; update/reintroduce submodules and .gitmodules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant IR as Interpreter
participant SP as PythSubParser (V2)
participant EX as PythExtern (IInterpreterExternV4)
participant OP as LibOpPythPrice
participant LP as LibPyth
participant PF as PythFeed
Note over IR,SP: Parse-time (OperandV2, bytes32[])
IR->>SP: Parse "pyth-price" word
SP-->>IR: OperandV2 + bytes32[] operand data
Note over IR,EX: Runtime (StackItem[])
IR->>EX: Execute opcode with OperandV2, StackItem[] inputs
EX->>OP: run(OperandV2, StackItem[])
OP->>LP: getPriceNoOlderThan(symbol, staleAfter: Float)
LP->>LP: toFixedDecimal(staleAfter) -> uint
LP->>PF: Query price with uint maxAge
PF-->>LP: Raw price + conf + expo
LP-->>OP: Return packed Floats (price, conf)
OP-->>EX: Return StackItem[] (price, conf)
EX-->>IR: StackItem[] result on stack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
src/generated/PythWords.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (6)
REUSE.toml(1 hunks)src/lib/op/LibOpPythPrice.sol(1 hunks)src/lib/pyth/LibPyth.sol(2 hunks)test/src/concrete/PythWords.pythPrice.t.sol(2 hunks)test/src/lib/op/LibOpPythPrice.t.sol(1 hunks)test/src/lib/pyth/LibPyth.getPriceNoOlderThan.t.sol(1 hunks)
🔇 Additional comments (7)
src/lib/pyth/LibPyth.sol (2)
132-143: Correct Float I/O; confirm staleAfter semantics and consider NatSpec update
- Converting Float maxAge to uint seconds with toFixedDecimalLossless(..., 0) and packing price/conf with the same expo is correct for Pyth.
- Please confirm toFixedDecimalLossless reverts on negative/non-integer values to avoid silent wrap to uint.
Also consider a brief NatSpec noting returns are (price, conf) as Floats with Pyth expo.
7-7: Import Float looks goodImporting Float alongside LibDecimalFloat aligns with the new Float-based API.
test/src/lib/op/LibOpPythPrice.t.sol (2)
12-16: Integrity update is correctExpecting 2 inputs and 2 outputs matches the new op contract.
21-29: Run-path assertions match op semanticsInputs are formed correctly (symbol, Float maxAge). Asserting outputs[0]=price and outputs[1]=conf aligns with LibOpPythPrice.run.
src/lib/op/LibOpPythPrice.sol (2)
15-17: Integrity matches new signatureFixed I/O arity (2,2) is appropriate.
21-27: Input decoding via assembly is soundReading symbol and staleAfter from inputs[0], inputs[1] (StackItem-wrapped bytes32) is correct.
test/src/lib/pyth/LibPyth.getPriceNoOlderThan.t.sol (1)
33-152: Fork-pinned expectations look good; keep pins updatedHard-coded expectations rely on FORK_BLOCK_* and submodule SHAs. If forks/SHAs change, values may drift. Ensure pins remain fixed or update expected values accordingly.
| StackItem[] memory outputs; | ||
| assembly ("memory-safe") { | ||
| outputs := mload(0x40) | ||
| mstore(0x40, add(outputs, 0x40)) | ||
| mstore(outputs, 1) | ||
| mstore(add(outputs, 0x20), price18) | ||
| mstore(0x40, add(outputs, 0x60)) | ||
| mstore(outputs, 2) | ||
| mstore(add(outputs, 0x20), price) | ||
| mstore(add(outputs, 0x40), conf) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Output array construction is correct; optional high-level alternative
The memory layout (len + 2 elements) and free memory pointer advance are correct.
Optional for readability:
- StackItem[] memory outputs;
- assembly ("memory-safe") {
- outputs := mload(0x40)
- mstore(0x40, add(outputs, 0x60))
- mstore(outputs, 2)
- mstore(add(outputs, 0x20), price)
- mstore(add(outputs, 0x40), conf)
- }
+ StackItem[] memory outputs = new StackItem[](2);
+ assembly ("memory-safe") {
+ mstore(add(outputs, 0x20), price)
+ mstore(add(outputs, 0x40), conf)
+ }📝 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.
| StackItem[] memory outputs; | |
| assembly ("memory-safe") { | |
| outputs := mload(0x40) | |
| mstore(0x40, add(outputs, 0x40)) | |
| mstore(outputs, 1) | |
| mstore(add(outputs, 0x20), price18) | |
| mstore(0x40, add(outputs, 0x60)) | |
| mstore(outputs, 2) | |
| mstore(add(outputs, 0x20), price) | |
| mstore(add(outputs, 0x40), conf) | |
| } | |
| StackItem[] memory outputs = new StackItem[](2); | |
| assembly ("memory-safe") { | |
| mstore(add(outputs, 0x20), price) | |
| mstore(add(outputs, 0x40), conf) | |
| } |
🤖 Prompt for AI Agents
In src/lib/op/LibOpPythPrice.sol around lines 31 to 38, the current inline
assembly that allocates and populates the outputs array is functionally correct;
if you prefer a clearer high-level alternative, replace the assembly with a
Solidity memory array allocation by creating a new StackItem[] of length 2,
assign the first element to price and the second to conf, and use that array as
outputs (removing the assembly block). Ensure the new allocation uses memory
(not storage) and that subsequent code expects the same two-element layout.
There was a problem hiding this comment.
that costs more gas
There was a problem hiding this comment.
@thedavidmeister, understood! The inline assembly approach is more gas-efficient. I'll note that the current implementation is optimal for performance.
| StackItem[] memory expectedStack = new StackItem[](2); | ||
| expectedStack[0] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(2.00302e5, -5))); | ||
| expectedStack[1] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(172.3176e5, -5))); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify stack order: LibOp returns [price, conf] but expectedStack is [conf, price]
LibOpPythPrice.run writes outputs[0]=price, outputs[1]=conf. If the DSL doesn’t reorder, expectedStack likely needs swapping.
Suggestion if needed:
-expectedStack[0] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(2.00302e5, -5)));
-expectedStack[1] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(172.3176e5, -5)));
+expectedStack[0] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(172.3176e5, -5)));
+expectedStack[1] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(2.00302e5, -5)));📝 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.
| StackItem[] memory expectedStack = new StackItem[](2); | |
| expectedStack[0] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(2.00302e5, -5))); | |
| expectedStack[1] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(172.3176e5, -5))); | |
| StackItem[] memory expectedStack = new StackItem[](2); | |
| expectedStack[0] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(172.3176e5, -5))); | |
| expectedStack[1] = StackItem.wrap(Float.unwrap(LibDecimalFloat.packLossless(2.00302e5, -5))); |
🤖 Prompt for AI Agents
In test/src/concrete/PythWords.pythPrice.t.sol around lines 24 to 27, the
expectedStack is currently [conf, price] but LibOpPythPrice.run returns outputs
in order [price, conf]; swap the two StackItem assignments so expectedStack[0]
is the price and expectedStack[1] is the confidence to match the operator output
ordering; ensure the values use the same LibDecimalFloat.packLossless calls but
placed in the corrected indices and update any comments to reflect the [price,
conf] ordering.
There was a problem hiding this comment.
@thedavidmeister, understood! Thanks for confirming—I'll note that the current stack order is correct as implemented.
| "price confidence: pyth-price(\"Equity.US.GOOG/USD\" 1080000),", | ||
| ":ensure(equal-to(price 172.3176) \"bad price\"),", | ||
| ":ensure(equal-to(confidence 2.00302) \"bad confidence\");" | ||
| ) | ||
| ), | ||
| expectedStack, | ||
| "pyth-price(\"Equity.US.GOOG/USD\" 10800)" | ||
| "pyth-price(\"Equity.US.GOOG/USD\" 1080000)" | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Confirm staleAfter units in DSL
The literal 1080000 should be seconds and effectively Float with expo 0. Confirm the DSL feeds a Float-equivalent into the extern (same as LibDecimalFloat.packLossless(x, 0)). Otherwise, prefer explicit Float packing for consistency.
🤖 Prompt for AI Agents
In test/src/concrete/PythWords.pythPrice.t.sol around lines 33 to 40, the
literal 1080000 (staleAfter) must be a seconds-based Float with exponent 0;
confirm the DSL is passing a Float-equivalent (i.e., packed the same as
LibDecimalFloat.packLossless(x, 0)). If the DSL does not auto-pack to Float with
expo 0, change the test to explicitly feed a packed Float (use the same
helper/packLossless call or the DSL’s explicit Float constructor) so the extern
receives a Float-equivalent value with exponent 0 for 1080000; update the test
assertion input accordingly to ensure consistency.
| function checkPriceNoOlderThan(IntOrAString symbol, Float maxAge, Float expectedPrice, Float expectedConf) | ||
| internal | ||
| view | ||
| { | ||
| (Float actualPrice, Float actualConf) = LibPyth.getPriceNoOlderThan(symbol, maxAge); | ||
| (int256 actualSignedCoefficient, int256 actualExponent) = LibDecimalFloat.unpack(actualPrice); | ||
| console2.logInt(actualSignedCoefficient); | ||
| console2.logInt(actualExponent); | ||
| (actualSignedCoefficient, actualExponent) = LibDecimalFloat.unpack(actualConf); | ||
| console2.logInt(actualSignedCoefficient); | ||
| console2.logInt(actualExponent); | ||
| assertEq(Float.unwrap(actualPrice), Float.unwrap(expectedPrice)); | ||
| assertEq(Float.unwrap(actualConf), Float.unwrap(expectedConf)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tidy verbose logging
Tests are correct; consider gating console2 logs to reduce noise:
- (int256 actualSignedCoefficient, int256 actualExponent) = LibDecimalFloat.unpack(actualPrice);
- console2.logInt(actualSignedCoefficient);
- console2.logInt(actualExponent);
- (actualSignedCoefficient, actualExponent) = LibDecimalFloat.unpack(actualConf);
- console2.logInt(actualSignedCoefficient);
- console2.logInt(actualExponent);
+ // if (false) {
+ // (int256 c, int256 e) = LibDecimalFloat.unpack(actualPrice);
+ // console2.logInt(c);
+ // console2.logInt(e);
+ // (c, e) = LibDecimalFloat.unpack(actualConf);
+ // console2.logInt(c);
+ // console2.logInt(e);
+ // }📝 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 checkPriceNoOlderThan(IntOrAString symbol, Float maxAge, Float expectedPrice, Float expectedConf) | |
| internal | |
| view | |
| { | |
| (Float actualPrice, Float actualConf) = LibPyth.getPriceNoOlderThan(symbol, maxAge); | |
| (int256 actualSignedCoefficient, int256 actualExponent) = LibDecimalFloat.unpack(actualPrice); | |
| console2.logInt(actualSignedCoefficient); | |
| console2.logInt(actualExponent); | |
| (actualSignedCoefficient, actualExponent) = LibDecimalFloat.unpack(actualConf); | |
| console2.logInt(actualSignedCoefficient); | |
| console2.logInt(actualExponent); | |
| assertEq(Float.unwrap(actualPrice), Float.unwrap(expectedPrice)); | |
| assertEq(Float.unwrap(actualConf), Float.unwrap(expectedConf)); | |
| } | |
| function checkPriceNoOlderThan(IntOrAString symbol, Float maxAge, Float expectedPrice, Float expectedConf) | |
| internal | |
| view | |
| { | |
| (Float actualPrice, Float actualConf) = LibPyth.getPriceNoOlderThan(symbol, maxAge); | |
| // if (false) { | |
| // (int256 c, int256 e) = LibDecimalFloat.unpack(actualPrice); | |
| // console2.logInt(c); | |
| // console2.logInt(e); | |
| // (c, e) = LibDecimalFloat.unpack(actualConf); | |
| // console2.logInt(c); | |
| // console2.logInt(e); | |
| // } | |
| assertEq(Float.unwrap(actualPrice), Float.unwrap(expectedPrice)); | |
| assertEq(Float.unwrap(actualConf), Float.unwrap(expectedConf)); | |
| } |
🤖 Prompt for AI Agents
In test/src/lib/pyth/LibPyth.getPriceNoOlderThan.t.sol around lines 18–31, the
test emits multiple console2.logInt calls unconditionally which creates noisy
test output; wrap those console2.logInt calls behind a verbosity flag (e.g., a
DEBUG/VERBOSE constant or an env-driven vm.envBool check) so they only run when
verbose logging is enabled, or remove them if they’re no longer needed, keeping
assertions intact.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/lib/pyth/LibPyth.sol(2 hunks)
⏰ 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: git-clean
- GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
- GitHub Check: rainix (ubuntu-latest, rainix-sol-legal)
🔇 Additional comments (4)
src/lib/pyth/LibPyth.sol (4)
7-7: LGTM: Float type import added.The addition of the
Floattype to the import statement is necessary for the updated function signature and return types.
137-139: LGTM: Improved confidence handling with Slither suppression.The addition of the Slither suppression comment and the return of confidence to the caller is a good improvement. Rather than ignoring the confidence interval internally, the function now returns it as the second
Floatvalue, allowing callers to decide how to handle price uncertainty. This addresses the previous Slither warning appropriately.
132-132: All getPriceNoOlderThan call sites updated
Callers now pass aFloatforstaleAfterand unpack twoFloatreturn values (e.g., in LibOpPythPrice.sol and tests).
133-133: Confirm fractional‐input handling forstaleAfter
toFixedDecimalLossless(staleAfter, 0)will revert ifstaleAfterhas any fractional component. Verify this is intended—if so, document thatstaleAftermust be a whole number or add an explicitrequirewith a clear error message.
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Refactor
Tests
Chores