Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pulse): add pulse contracts #2090

Merged
merged 28 commits into from
Jan 20, 2025
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
911887f
add initial contracts
cctdaniel Nov 4, 2024
d05d869
refactor
cctdaniel Nov 4, 2024
f017f29
fix
cctdaniel Nov 6, 2024
19ae7ea
fix test
cctdaniel Nov 6, 2024
fbbae70
fix test
cctdaniel Nov 6, 2024
061dfb2
fix
cctdaniel Nov 7, 2024
ccaab98
fix
cctdaniel Nov 11, 2024
27f665b
add more tests
cctdaniel Nov 12, 2024
035938f
add test for getFee
cctdaniel Nov 13, 2024
a7a4d23
add testWithdraw
cctdaniel Nov 13, 2024
183658e
add testSetAndWithdrawAssFeeManager
cctdaniel Nov 13, 2024
e957c79
add testMaxNumPrices
cctdaniel Nov 13, 2024
54d310e
add testSetProviderUri
cctdaniel Nov 13, 2024
2ca191a
add more test
cctdaniel Nov 13, 2024
c455937
add testExecuteCallbackWithFutureTimestamp
cctdaniel Nov 13, 2024
b313728
update tests
cctdaniel Nov 13, 2024
8f64744
remove provider
cctdaniel Nov 18, 2024
bce0770
address comments
cctdaniel Nov 18, 2024
9d5cd7e
address comments
cctdaniel Nov 18, 2024
81e4535
prevent requests 60 mins in the future that could exploit gas price d…
cctdaniel Jan 6, 2025
cda5a2b
fix test
cctdaniel Jan 6, 2025
604b113
add priceIds to PriceUpdateRequested event
cctdaniel Jan 6, 2025
dedca30
add 50% overhead to gas for cross-contract calls
cctdaniel Jan 7, 2025
d748162
feat: add test for executing callback with gas overhead
cctdaniel Jan 7, 2025
5399eb0
feat: add docs for requestPriceUpdatesWithCallback and executeCallbac…
cctdaniel Jan 7, 2025
7cd8328
fix: use fixed-length array for priceIds in req
cctdaniel Jan 8, 2025
9e046dc
add test
cctdaniel Jan 8, 2025
1f82d5e
address comments
cctdaniel Jan 17, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions target_chains/ethereum/contracts/contracts/pulse/IPulse.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// SPDX-License-Identifier: Apache 2

pragma solidity ^0.8.0;

import "@pythnetwork/pyth-sdk-solidity/IPyth.sol";
import "./PulseEvents.sol";
import "./PulseState.sol";

interface IPulseConsumer {
function pulseCallback(
cctdaniel marked this conversation as resolved.
Show resolved Hide resolved
uint64 sequenceNumber,
address updater,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have provider but if you don't have provider i don't think you need this parameter either.

PythStructs.PriceFeed[] memory priceFeeds
) external;
}

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
cctdaniel marked this conversation as resolved.
Show resolved Hide resolved
* 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
cctdaniel marked this conversation as resolved.
Show resolved Hide resolved
* @param priceIds The price feed IDs to update
* @param callbackGasLimit Gas limit for the callback execution
* @return sequenceNumber The sequence number assigned to this request
*/
function requestPriceUpdatesWithCallback(
uint256 publishTime,
bytes32[] calldata priceIds,
uint256 callbackGasLimit
) external payable returns (uint64 sequenceNumber);

/**
* @notice Executes the callback for a price update request
* @dev Requires 1.5x the callback gas limit to account for cross-contract call overhead
* For example, if callbackGasLimit is 1M, the transaction needs at least 1.5M gas + some gas for some other operations in the function before the callback
* @param sequenceNumber The sequence number of the request
* @param updateData The raw price update data from Pyth
* @param priceIds The price feed IDs to update, must match the request
*/
function executeCallback(
uint64 sequenceNumber,
bytes[] calldata updateData,
bytes32[] calldata priceIds
) external payable;

// Getters
function getFee(
uint256 callbackGasLimit
) external view returns (uint128 feeAmount);

function getPythFeeInWei() external view returns (uint128 pythFeeInWei);
cctdaniel marked this conversation as resolved.
Show resolved Hide resolved

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

function getRequest(
uint64 sequenceNumber
) external view returns (PulseState.Request memory req);

// Add these functions to the IPulse interface
function setFeeManager(address manager) external;

function withdrawAsFeeManager(uint128 amount) external;
}
282 changes: 282 additions & 0 deletions target_chains/ethereum/contracts/contracts/pulse/Pulse.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,282 @@
// SPDX-License-Identifier: Apache 2

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/utils/math/SafeCast.sol";
import "@pythnetwork/pyth-sdk-solidity/IPyth.sol";
import "./IPulse.sol";
import "./PulseState.sol";
import "./PulseErrors.sol";

abstract contract Pulse is IPulse, PulseState {
function _initialize(
address admin,
uint128 pythFeeInWei,
address pythAddress,
bool prefillRequestStorage
) internal {
require(admin != address(0), "admin is zero address");
require(pythAddress != address(0), "pyth is zero address");

_state.admin = admin;
_state.accruedFeesInWei = 0;
_state.pythFeeInWei = pythFeeInWei;
_state.pyth = pythAddress;
_state.currentSequenceNumber = 1;

if (prefillRequestStorage) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this for :?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also referenced from the entropy contract -- which is to optimize for gas costs, by pre-filling the storage slots with non-zero values during initialization we pay the high gas cost once during deployment and hence subsequent writes to these slots will cost less gas

for (uint8 i = 0; i < NUM_REQUESTS; i++) {
Request storage req = _state.requests[i];
req.sequenceNumber = 0;
req.publishTime = 1;
req.callbackGasLimit = 1;
req.requester = address(1);
req.numPriceIds = 0;
// Pre-warm the priceIds array storage
for (uint8 j = 0; j < MAX_PRICE_IDS; j++) {
req.priceIds[j] = bytes32(0);
}
}
}
}

function requestPriceUpdatesWithCallback(
uint256 publishTime,
bytes32[] calldata priceIds,
uint256 callbackGasLimit
) external payable override returns (uint64 requestSequenceNumber) {
require(publishTime <= block.timestamp + 60, "Too far in future");
cctdaniel marked this conversation as resolved.
Show resolved Hide resolved
if (priceIds.length > MAX_PRICE_IDS) {
revert TooManyPriceIds(priceIds.length, MAX_PRICE_IDS);
}
requestSequenceNumber = _state.currentSequenceNumber++;

uint128 requiredFee = getFee(callbackGasLimit);
if (msg.value < requiredFee) revert InsufficientFee();

Request storage req = allocRequest(requestSequenceNumber);
req.sequenceNumber = requestSequenceNumber;
req.publishTime = publishTime;
req.callbackGasLimit = callbackGasLimit;
req.requester = msg.sender;
req.numPriceIds = uint8(priceIds.length);

// Copy price IDs to storage
for (uint8 i = 0; i < priceIds.length; i++) {
req.priceIds[i] = priceIds[i];
}

_state.accruedFeesInWei += SafeCast.toUint128(msg.value);

emit PriceUpdateRequested(req, priceIds);
}

function executeCallback(
uint64 sequenceNumber,
bytes[] calldata updateData,
bytes32[] calldata priceIds
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm thinking about it, now that we don't emit the ids and don't store them (we store the hash of it), how argus is supposed to understand it :? parse the transactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i think we should still emit the event with the priceId and the trade-off of higher gas costs for event emission is necessary because the system can't function without Argus having access to the price IDs, we can keep the storage the hash still so that it remains optimized, what do you think?

) external payable override {
Request storage req = findActiveRequest(sequenceNumber);

// Verify priceIds match
require(
priceIds.length == req.numPriceIds,
"Price IDs length mismatch"
);
for (uint8 i = 0; i < req.numPriceIds; i++) {
if (priceIds[i] != req.priceIds[i]) {
revert InvalidPriceIds(priceIds[i], req.priceIds[i]);
}
}

// Parse price feeds first to measure gas usage
PythStructs.PriceFeed[] memory priceFeeds = IPyth(_state.pyth)
.parsePriceFeedUpdates(
cctdaniel marked this conversation as resolved.
Show resolved Hide resolved
updateData,
priceIds,
cctdaniel marked this conversation as resolved.
Show resolved Hide resolved
SafeCast.toUint64(req.publishTime),
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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think 50% overhead is too much, the overhead is mostly constant.

if (gasleft() < (req.callbackGasLimit * 3) / 2) {
revert InsufficientGas();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need this check? Why not just run the rest of the code, and then either you run out of gas or you don't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it just provides better UX by failing early with a clear error message, also actual gas paid is gas used

there might be a situation where:

  • user provides exactly callbackGasLimit to pulseCallback
  • gasleft() is also callbackGasLimit
  • we don't have enough gas for events/cleanup
  • transaction fails with generic out-of-gas error


try
IPulseConsumer(req.requester).pulseCallback{
gas: req.callbackGasLimit
}(sequenceNumber, msg.sender, priceFeeds)
{
// Callback succeeded
emitPriceUpdate(sequenceNumber, priceIds, priceFeeds);
} catch Error(string memory reason) {
// Explicit revert/require
emit PriceUpdateCallbackFailed(
sequenceNumber,
msg.sender,
priceIds,
req.requester,
reason
);
} catch {
// Out of gas or other low-level errors
emit PriceUpdateCallbackFailed(
sequenceNumber,
msg.sender,
priceIds,
req.requester,
"low-level error (possibly out of gas)"
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you want these catches either. If either of these errors happens for a transient reason, it means the callback can't be called in the future.

In entropy if a callback fails, we leave the request open. It could get fulfilled later, or it could just sit there forever.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this is actually important. In practice, we have callbacks that error on the first try and then succeed on a later retry)

Copy link
Contributor

@jayantk jayantk Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you also need to use the Check-Effects-Interactions pattern here, meaning the clearRequest call (the effect) needs to happen before cross-contract calls (interactions). There's a whole class of security vulnerabilities if you don't do the interactions last, because the cross-contract call can call back into the Pulse contract and then weird stuff can happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason why we have this try-catch is because @m30m said that in entropy it's difficult to know if its the user error or our error so logging this will provide better UX, we can document that the callback contract should not fail, and if it fails we wont retry


clearRequest(sequenceNumber);
}

function emitPriceUpdate(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to emit something (for debugging purposes), just be aware that it might be expensive to do this in ethereum mainnet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed but apart from debugging this is also useful for off-chain indexing/integration so i think the benefits outweigh the costs

uint64 sequenceNumber,
bytes32[] memory priceIds,
PythStructs.PriceFeed[] memory priceFeeds
) internal {
int64[] memory prices = new int64[](priceFeeds.length);
uint64[] memory conf = new uint64[](priceFeeds.length);
int32[] memory expos = new int32[](priceFeeds.length);
uint256[] memory publishTimes = new uint256[](priceFeeds.length);

for (uint i = 0; i < priceFeeds.length; i++) {
prices[i] = priceFeeds[i].price.price;
conf[i] = priceFeeds[i].price.conf;
expos[i] = priceFeeds[i].price.expo;
publishTimes[i] = priceFeeds[i].price.publishTime;
}

emit PriceUpdateExecuted(
sequenceNumber,
msg.sender,
priceIds,
prices,
conf,
expos,
publishTimes
);
}

function getFee(
uint256 callbackGasLimit
) public view override returns (uint128 feeAmount) {
uint128 baseFee = _state.pythFeeInWei;
uint256 gasFee = callbackGasLimit * tx.gasprice;
feeAmount = baseFee + SafeCast.toUint128(gasFee);
}
Comment on lines +173 to +179
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how this works in Entropy. Do we update pythFee regularly based on gasPrice? @m30m might know better but similar products charge fee as % of the tx fee.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also one DoS attack vector in our price feeds use case regarding gas prices. People can ask for price updates anytime in the future and it won't be fullfilled immediately (opposed to Entropy) and this might make tx.gasprice a worse estimate on normal usage (only a few seconds after that) or can be taken advantage of to make hundreds of requests in the future when the gas price is lower.

My recommendation is to not allow a publish time that is more than 1min in the future. Or charge higher fees as it goes more into the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that's a good point, i think not allowing a publish time more than 1 min in the future is a reasonable solution

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Entropy, the keeper updates its fee over time.

I think the thing to do here is to give the keeper a configurable feePerGas parameter instead of tx.gasprice. The keeper then sets both a base fee and the feePerGas so it's a linear ax+b pricing model and then we can figure out how to do this properly off chain.


function getPythFeeInWei()
public
view
override
returns (uint128 pythFeeInWei)
{
pythFeeInWei = _state.pythFeeInWei;
}

function getAccruedFees()
public
view
override
returns (uint128 accruedFeesInWei)
{
accruedFeesInWei = _state.accruedFeesInWei;
}

function getRequest(
uint64 sequenceNumber
) public view override returns (Request memory req) {
req = findRequest(sequenceNumber);
}

function requestKey(
uint64 sequenceNumber
) internal pure returns (bytes32 hash, uint8 shortHash) {
hash = keccak256(abi.encodePacked(sequenceNumber));
shortHash = uint8(hash[0] & NUM_REQUESTS_MASK);
}

function withdrawFees(uint128 amount) external {
require(msg.sender == _state.admin, "Only admin can withdraw fees");
require(_state.accruedFeesInWei >= amount, "Insufficient balance");

_state.accruedFeesInWei -= amount;

(bool sent, ) = msg.sender.call{value: amount}("");
require(sent, "Failed to send fees");

emit FeesWithdrawn(msg.sender, amount);
}

function findActiveRequest(
uint64 sequenceNumber
) internal view returns (Request storage req) {
req = findRequest(sequenceNumber);

if (!isActive(req) || req.sequenceNumber != sequenceNumber)
revert NoSuchRequest();
}

function findRequest(
uint64 sequenceNumber
) internal view returns (Request storage req) {
(bytes32 key, uint8 shortKey) = requestKey(sequenceNumber);

req = _state.requests[shortKey];
if (req.sequenceNumber == sequenceNumber) {
return req;
} else {
req = _state.requestsOverflow[key];
}
}

function clearRequest(uint64 sequenceNumber) internal {
(bytes32 key, uint8 shortKey) = requestKey(sequenceNumber);

Request storage req = _state.requests[shortKey];
if (req.sequenceNumber == sequenceNumber) {
req.sequenceNumber = 0;
} else {
delete _state.requestsOverflow[key];
}
}

function allocRequest(
uint64 sequenceNumber
) internal returns (Request storage req) {
(, uint8 shortKey) = requestKey(sequenceNumber);

req = _state.requests[shortKey];
if (isActive(req)) {
(bytes32 reqKey, ) = requestKey(req.sequenceNumber);
_state.requestsOverflow[reqKey] = req;
}
}

function isActive(Request storage req) internal view returns (bool) {
return req.sequenceNumber != 0;
}

function setFeeManager(address manager) external override {
require(msg.sender == _state.admin, "Only admin can set fee manager");
address oldFeeManager = _state.feeManager;
_state.feeManager = manager;
emit FeeManagerUpdated(_state.admin, oldFeeManager, manager);
}

function withdrawAsFeeManager(uint128 amount) external override {
require(msg.sender == _state.feeManager, "Only fee manager");
require(_state.accruedFeesInWei >= amount, "Insufficient balance");

_state.accruedFeesInWei -= amount;

(bool sent, ) = msg.sender.call{value: amount}("");
require(sent, "Failed to send fees");

emit FeesWithdrawn(msg.sender, amount);
}
}
15 changes: 15 additions & 0 deletions target_chains/ethereum/contracts/contracts/pulse/PulseErrors.sol
cctdaniel marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: Apache 2

pragma solidity ^0.8.0;

error NoSuchProvider();
error NoSuchRequest();
error InsufficientFee();
error Unauthorized();
error InvalidCallbackGas();
error CallbackFailed();
error InvalidPriceIds(bytes32 providedPriceIdsHash, bytes32 storedPriceIdsHash);
error InvalidCallbackGasLimit(uint256 requested, uint256 stored);
error ExceedsMaxPrices(uint32 requested, uint32 maxAllowed);
error InsufficientGas();
error TooManyPriceIds(uint256 provided, uint256 maximum);
Loading
Loading