Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
cctdaniel committed Jan 17, 2025
1 parent 9e046dc commit 1f82d5e
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 12 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@ __pycache__
.direnv
.next
.turbo/
.cursorrules
31 changes: 23 additions & 8 deletions target_chains/ethereum/contracts/contracts/pulse/IPulse.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,17 @@ interface IPulse is PulseEvents {
// Core functions
/**
* @notice Requests price updates with a callback
* @dev The msg.value must cover both the Pyth fee and gas costs
* Note: The actual gas required for execution will be 1.5x the callbackGasLimit
* to account for cross-contract call overhead + some gas for some other operations in the function before the callback
* @param publishTime The minimum publish time for price updates
* @param priceIds The price feed IDs to update
* @param callbackGasLimit Gas limit for the callback execution
* @dev The msg.value must be equal to getFee(callbackGasLimit)
* @param callbackGasLimit The amount of gas allocated for the callback execution
* @param publishTime The minimum publish time for price updates, it should be less than or equal to block.timestamp + 60
* @param priceIds The price feed IDs to update. Maximum 10 price feeds per request.
* Requests requiring more feeds should be split into multiple calls.
* @return sequenceNumber The sequence number assigned to this request
* @dev Security note: The 60-second future limit on publishTime prevents a DoS vector where
* attackers could submit many low-fee requests for far-future updates when gas prices
* are low, forcing executors to fulfill them later when gas prices might be much higher.
* Since tx.gasprice is used to calculate fees, allowing far-future requests would make
* the fee estimation unreliable.
*/
function requestPriceUpdatesWithCallback(
uint256 publishTime,
Expand All @@ -47,12 +51,23 @@ interface IPulse is PulseEvents {
) external payable;

// Getters
/**
* @notice Gets the base fee charged by Pyth protocol
* @dev This is a fixed fee per request that goes to the Pyth protocol, separate from gas costs
* @return pythFeeInWei The base fee in wei that every request must pay
*/
function getPythFeeInWei() external view returns (uint128 pythFeeInWei);

/**
* @notice Calculates the total fee required for a price update request
* @dev Total fee = base Pyth protocol fee + gas costs for callback
* @param callbackGasLimit The amount of gas allocated for callback execution
* @return feeAmount The total fee in wei that must be provided as msg.value
*/
function getFee(
uint256 callbackGasLimit
) external view returns (uint128 feeAmount);

function getPythFeeInWei() external view returns (uint128 pythFeeInWei);

function getAccruedFees() external view returns (uint128 accruedFeesInWei);

function getRequest(
Expand Down
15 changes: 12 additions & 3 deletions target_chains/ethereum/contracts/contracts/pulse/Pulse.sol
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ abstract contract Pulse is IPulse, PulseState {
bytes32[] calldata priceIds,
uint256 callbackGasLimit
) external payable override returns (uint64 requestSequenceNumber) {
// NOTE: The 60-second future limit on publishTime prevents a DoS vector where
// attackers could submit many low-fee requests for far-future updates when gas prices
// are low, forcing executors to fulfill them later when gas prices might be much higher.
// Since tx.gasprice is used to calculate fees, allowing far-future requests would make
// the fee estimation unreliable.
require(publishTime <= block.timestamp + 60, "Too far in future");
if (priceIds.length > MAX_PRICE_IDS) {
revert TooManyPriceIds(priceIds.length, MAX_PRICE_IDS);
Expand Down Expand Up @@ -98,7 +103,13 @@ abstract contract Pulse is IPulse, PulseState {
SafeCast.toUint64(req.publishTime)
);

// Check if enough gas remains for the callback plus 50% overhead for cross-contract call (uses integer arithmetic to avoid floating point)
clearRequest(sequenceNumber);

// Check if enough gas remains for callback + events/cleanup
// We need extra gas beyond callbackGasLimit for:
// 1. Emitting success/failure events
// 2. Error handling in catch blocks
// 3. State cleanup operations
if (gasleft() < (req.callbackGasLimit * 3) / 2) {
revert InsufficientGas();
}
Expand Down Expand Up @@ -129,8 +140,6 @@ abstract contract Pulse is IPulse, PulseState {
"low-level error (possibly out of gas)"
);
}

clearRequest(sequenceNumber);
}

function emitPriceUpdate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ pragma solidity ^0.8.0;
contract PulseState {
uint8 public constant NUM_REQUESTS = 32;
bytes1 public constant NUM_REQUESTS_MASK = 0x1f;
// Maximum number of price feeds per request. This limit keeps gas costs predictable and reasonable. 10 is a reasonable number for most use cases.
// Requests with more than 10 price feeds should be split into multiple requests
uint8 public constant MAX_PRICE_IDS = 10;

struct Request {
Expand All @@ -23,7 +25,7 @@ contract PulseState {
address pyth;
uint64 currentSequenceNumber;
address feeManager;
Request[32] requests;
Request[NUM_REQUESTS] requests;
mapping(bytes32 => Request) requestsOverflow;
}

Expand Down

0 comments on commit 1f82d5e

Please sign in to comment.