From c3461e5e1cfc01fa0dda0b2006a0b762b982af50 Mon Sep 17 00:00:00 2001 From: Ali Behjati Date: Tue, 29 Nov 2022 18:30:45 +0100 Subject: [PATCH] [eth] Use PythErrors everywhere (#404) * Remove unnecessary check * Use PythErrors everywhere --- ethereum/contracts/pyth/Pyth.sol | 100 ++++++---------- ethereum/contracts/pyth/PythGovernance.sol | 69 +++++------ .../pyth/PythGovernanceInstructions.sol | 40 +++---- ethereum/contracts/pyth/PythUpgradable.sol | 10 +- ethereum/forge-test/GasBenchmark.t.sol | 13 +- ethereum/forge-test/Pyth.t.sol | 19 +-- ethereum/package-lock.json | 14 +-- ethereum/package.json | 2 +- ethereum/test/pyth.js | 111 ++++++++++-------- 9 files changed, 164 insertions(+), 214 deletions(-) diff --git a/ethereum/contracts/pyth/Pyth.sol b/ethereum/contracts/pyth/Pyth.sol index 4e7d7dd3..552594d9 100644 --- a/ethereum/contracts/pyth/Pyth.sol +++ b/ethereum/contracts/pyth/Pyth.sol @@ -7,6 +7,7 @@ import "../libraries/external/UnsafeBytesLib.sol"; import "@pythnetwork/pyth-sdk-solidity/AbstractPyth.sol"; import "@pythnetwork/pyth-sdk-solidity/PythStructs.sol"; +import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol"; import "./PythGetters.sol"; import "./PythSetters.sol"; import "./PythInternalStructs.sol"; @@ -21,11 +22,10 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { ) internal { setWormhole(wormhole); - require( - dataSourceEmitterChainIds.length == - dataSourceEmitterAddresses.length, - "data source arguments should have the same length" - ); + if ( + dataSourceEmitterChainIds.length != + dataSourceEmitterAddresses.length + ) revert PythErrors.InvalidArgument(); for (uint i = 0; i < dataSourceEmitterChainIds.length; i++) { PythInternalStructs.DataSource memory ds = PythInternalStructs @@ -34,10 +34,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { dataSourceEmitterAddresses[i] ); - require( - !PythGetters.isValidDataSource(ds.chainId, ds.emitterAddress), - "Data source already added" - ); + if (PythGetters.isValidDataSource(ds.chainId, ds.emitterAddress)) + revert PythErrors.InvalidArgument(); _state.isValidDataSource[hashDataSource(ds)] = true; _state.validDataSources.push(ds); @@ -57,7 +55,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { bytes[] calldata updateData ) public payable override { uint requiredFee = getUpdateFee(updateData); - require(msg.value >= requiredFee, "insufficient paid fee amount"); + if (msg.value < requiredFee) revert PythErrors.InsufficientFee(); for (uint i = 0; i < updateData.length; ) { updatePriceBatchFromVm(updateData[i]); @@ -245,10 +243,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { } } - require( - attestationIndex <= attestationSize, - "INTERNAL: Consumed more than `attestationSize` bytes" - ); + if (attestationIndex > attestationSize) + revert PythErrors.InvalidUpdateData(); } } @@ -259,10 +255,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { bytes32[] calldata priceIds, uint64[] calldata publishTimes ) external payable override { - require( - priceIds.length == publishTimes.length, - "priceIds and publishTimes arrays should have same length" - ); + if (priceIds.length != publishTimes.length) + revert PythErrors.InvalidArgument(); for (uint i = 0; i < priceIds.length; ) { // If the price does not exist, then the publish time is zero and @@ -277,9 +271,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { } } - revert( - "no prices in the submitted batch have fresh prices, so this update will have no effect" - ); + revert PythErrors.NoFreshUpdate(); } // This is an overwrite of the same method in AbstractPyth.sol @@ -295,10 +287,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { price.price = info.price; price.conf = info.conf; - require( - price.publishTime != 0, - "price feed for the given id is not pushed or does not exist" - ); + if (price.publishTime == 0) revert PythErrors.PriceFeedNotFound(); } // This is an overwrite of the same method in AbstractPyth.sol @@ -314,10 +303,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { price.price = info.emaPrice; price.conf = info.emaConf; - require( - price.publishTime != 0, - "price feed for the given id is not pushed or does not exist" - ); + if (price.publishTime == 0) revert PythErrors.PriceFeedNotFound(); } function parseBatchAttestationHeader( @@ -334,18 +320,20 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { { uint32 magic = UnsafeBytesLib.toUint32(encoded, index); index += 4; - require(magic == 0x50325748, "invalid magic value"); + if (magic != 0x50325748) revert PythErrors.InvalidUpdateData(); uint16 versionMajor = UnsafeBytesLib.toUint16(encoded, index); index += 2; - require(versionMajor == 3, "invalid version major, expected 3"); + if (versionMajor != 3) revert PythErrors.InvalidUpdateData(); - uint16 versionMinor = UnsafeBytesLib.toUint16(encoded, index); + // This value is only used as the check below which currently + // never reverts + // uint16 versionMinor = UnsafeBytesLib.toUint16(encoded, index); index += 2; - require( - versionMinor >= 0, - "invalid version minor, expected 0 or more" - ); + + // This check is always false as versionMinor is 0, so it is commented. + // in the future that the minor version increases this will have effect. + // if(versionMinor < 0) revert InvalidUpdateData(); uint16 hdrSize = UnsafeBytesLib.toUint16(encoded, index); index += 2; @@ -370,10 +358,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { index += hdrSize; // Payload ID of 2 required for batch headerBa - require( - payloadId == 2, - "invalid payload ID, expected 2 for BatchPriceAttestation" - ); + if (payloadId != 2) revert PythErrors.InvalidUpdateData(); } // Parse the number of attestations @@ -386,10 +371,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { // Given the message is valid the arithmetic below should not overflow, and // even if it overflows then the require would fail. - require( - encoded.length == (index + (attestationSize * nAttestations)), - "invalid BatchPriceAttestation size" - ); + if (encoded.length != (index + (attestationSize * nAttestations))) + revert PythErrors.InvalidUpdateData(); } } @@ -398,12 +381,11 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { ) internal view returns (IWormhole.VM memory vm) { { bool valid; - string memory reason; - (vm, valid, reason) = wormhole().parseAndVerifyVM(encodedVm); - require(valid, reason); + (vm, valid, ) = wormhole().parseAndVerifyVM(encodedVm); + if (!valid) revert PythErrors.InvalidWormholeVaa(); } - require(verifyPythVM(vm), "invalid data source chain/emitter ID"); + if (!verifyPythVM(vm)) revert PythErrors.InvalidUpdateDataSource(); } function parsePriceFeedUpdates( @@ -420,10 +402,8 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { unchecked { { uint requiredFee = getUpdateFee(updateData); - require( - msg.value >= requiredFee, - "insufficient paid fee amount" - ); + if (msg.value < requiredFee) + revert PythErrors.InsufficientFee(); } priceFeeds = new PythStructs.PriceFeed[](priceIds.length); @@ -482,10 +462,6 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { index, attestationSize ); - require( - info.publishTime != 0, - "price feed for the given id is not pushed or does not exist" - ); priceFeeds[k].id = priceId; priceFeeds[k].price.price = info.price; @@ -513,10 +489,9 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { } for (uint k = 0; k < priceIds.length; k++) { - require( - priceFeeds[k].id != 0, - "1 or more price feeds are not found in the updateData or they are out of the given time range" - ); + if (priceFeeds[k].id == 0) { + revert PythErrors.PriceFeedNotFoundWithinRange(); + } } } } @@ -526,10 +501,7 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth { ) public view override returns (PythStructs.PriceFeed memory priceFeed) { // Look up the latest price info for the given ID PythInternalStructs.PriceInfo memory info = latestPriceInfo(id); - require( - info.publishTime != 0, - "price feed for the given id is not pushed or does not exist" - ); + if (info.publishTime == 0) revert PythErrors.PriceFeedNotFound(); priceFeed.id = id; priceFeed.price.price = info.price; diff --git a/ethereum/contracts/pyth/PythGovernance.sol b/ethereum/contracts/pyth/PythGovernance.sol index e1740482..32030292 100644 --- a/ethereum/contracts/pyth/PythGovernance.sol +++ b/ethereum/contracts/pyth/PythGovernance.sol @@ -7,6 +7,7 @@ import "./PythGovernanceInstructions.sol"; import "./PythInternalStructs.sol"; import "./PythGetters.sol"; import "./PythSetters.sol"; +import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol"; import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Upgrade.sol"; @@ -37,19 +38,17 @@ abstract contract PythGovernance is function verifyGovernanceVM( bytes memory encodedVM ) internal returns (IWormhole.VM memory parsedVM) { - (IWormhole.VM memory vm, bool valid, string memory reason) = wormhole() - .parseAndVerifyVM(encodedVM); - - require(valid, reason); - require( - isValidGovernanceDataSource(vm.emitterChainId, vm.emitterAddress), - "VAA is not coming from the governance data source" + (IWormhole.VM memory vm, bool valid, ) = wormhole().parseAndVerifyVM( + encodedVM ); - require( - vm.sequence > lastExecutedGovernanceSequence(), - "VAA is older than the last executed governance VAA" - ); + if (!valid) revert PythErrors.InvalidWormholeVaa(); + + if (!isValidGovernanceDataSource(vm.emitterChainId, vm.emitterAddress)) + revert PythErrors.InvalidGovernanceDataSource(); + + if (vm.sequence <= lastExecutedGovernanceSequence()) + revert PythErrors.OldGovernanceMessage(); setLastExecutedGovernanceSequence(vm.sequence); @@ -63,16 +62,12 @@ abstract contract PythGovernance is vm.payload ); - require( - gi.targetChainId == chainId() || gi.targetChainId == 0, - "invalid target chain for this governance instruction" - ); + if (gi.targetChainId != chainId() && gi.targetChainId != 0) + revert PythErrors.InvalidGovernanceTarget(); if (gi.action == GovernanceAction.UpgradeContract) { - require( - gi.targetChainId != 0, - "upgrade with chain id 0 is not possible" - ); + if (gi.targetChainId == 0) + revert PythErrors.InvalidGovernanceTarget(); upgradeContract(parseUpgradeContractPayload(gi.payload)); } else if ( gi.action == GovernanceAction.AuthorizeGovernanceDataSourceTransfer @@ -89,11 +84,10 @@ abstract contract PythGovernance is } else if ( gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer ) { - revert( - "RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message" - ); + // RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message + revert PythErrors.InvalidGovernanceMessage(); } else { - revert("invalid governance action"); + revert PythErrors.InvalidGovernanceMessage(); } } @@ -119,21 +113,19 @@ abstract contract PythGovernance is // If it's valid then its emitter can take over the governance from the current emitter. // The VAA is checked here to ensure that the new governance data source is valid and can send message // through wormhole. - (IWormhole.VM memory vm, bool valid, string memory reason) = wormhole() - .parseAndVerifyVM(payload.claimVaa); - require(valid, reason); + (IWormhole.VM memory vm, bool valid, ) = wormhole().parseAndVerifyVM( + payload.claimVaa + ); + if (!valid) revert PythErrors.InvalidWormholeVaa(); GovernanceInstruction memory gi = parseGovernanceInstruction( vm.payload ); - require( - gi.targetChainId == chainId() || gi.targetChainId == 0, - "invalid target chain for this governance instruction" - ); - require( - gi.action == GovernanceAction.RequestGovernanceDataSourceTransfer, - "governance data source change inner vaa is not of claim action type" - ); + if (gi.targetChainId != chainId() && gi.targetChainId != 0) + revert PythErrors.InvalidGovernanceTarget(); + + if (gi.action != GovernanceAction.RequestGovernanceDataSourceTransfer) + revert PythErrors.InvalidGovernanceMessage(); RequestGovernanceDataSourceTransferPayload memory claimPayload = parseRequestGovernanceDataSourceTransferPayload( @@ -141,11 +133,10 @@ abstract contract PythGovernance is ); // Governance data source index is used to prevent replay attacks, so a claimVaa cannot be used twice. - require( - governanceDataSourceIndex() < - claimPayload.governanceDataSourceIndex, - "cannot upgrade to an older governance data source" - ); + if ( + governanceDataSourceIndex() >= + claimPayload.governanceDataSourceIndex + ) revert PythErrors.OldGovernanceMessage(); setGovernanceDataSourceIndex(claimPayload.governanceDataSourceIndex); diff --git a/ethereum/contracts/pyth/PythGovernanceInstructions.sol b/ethereum/contracts/pyth/PythGovernanceInstructions.sol index 1c7e398c..f70218d6 100644 --- a/ethereum/contracts/pyth/PythGovernanceInstructions.sol +++ b/ethereum/contracts/pyth/PythGovernanceInstructions.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.0; import "../libraries/external/BytesLib.sol"; import "./PythInternalStructs.sol"; +import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol"; /** * @dev `PythGovernanceInstructions` defines a set of structs and parsing functions @@ -75,17 +76,16 @@ contract PythGovernanceInstructions { uint index = 0; uint32 magic = encodedInstruction.toUint32(index); - require(magic == MAGIC, "invalid magic for GovernanceInstruction"); + + if (magic != MAGIC) revert PythErrors.InvalidGovernanceMessage(); + index += 4; uint8 modNumber = encodedInstruction.toUint8(index); gi.module = GovernanceModule(modNumber); index += 1; - require( - gi.module == MODULE, - "invalid module for GovernanceInstruction" - ); + if (gi.module != MODULE) revert PythErrors.InvalidGovernanceTarget(); uint8 actionNumber = encodedInstruction.toUint8(index); gi.action = GovernanceAction(actionNumber); @@ -112,10 +112,8 @@ contract PythGovernanceInstructions { uc.newImplementation = address(encodedPayload.toAddress(index)); index += 20; - require( - encodedPayload.length == index, - "invalid length for UpgradeContractPayload" - ); + if (encodedPayload.length != index) + revert PythErrors.InvalidGovernanceMessage(); } /// @dev Parse a AuthorizeGovernanceDataSourceTransferPayload (action 2) with minimal validation @@ -142,10 +140,8 @@ contract PythGovernanceInstructions { sgdsClaim.governanceDataSourceIndex = encodedPayload.toUint32(index); index += 4; - require( - encodedPayload.length == index, - "invalid length for RequestGovernanceDataSourceTransferPayload" - ); + if (encodedPayload.length != index) + revert PythErrors.InvalidGovernanceMessage(); } /// @dev Parse a SetDataSourcesPayload (action 3) with minimal validation @@ -169,10 +165,8 @@ contract PythGovernanceInstructions { index += 32; } - require( - encodedPayload.length == index, - "invalid length for SetDataSourcesPayload" - ); + if (encodedPayload.length != index) + revert PythErrors.InvalidGovernanceMessage(); } /// @dev Parse a SetFeePayload (action 4) with minimal validation @@ -189,10 +183,8 @@ contract PythGovernanceInstructions { sf.newFee = uint256(val) * uint256(10) ** uint256(expo); - require( - encodedPayload.length == index, - "invalid length for SetFeePayload" - ); + if (encodedPayload.length != index) + revert PythErrors.InvalidGovernanceMessage(); } /// @dev Parse a SetValidPeriodPayload (action 5) with minimal validation @@ -204,9 +196,7 @@ contract PythGovernanceInstructions { svp.newValidPeriod = uint256(encodedPayload.toUint64(index)); index += 8; - require( - encodedPayload.length == index, - "invalid length for SetValidPeriodPayload" - ); + if (encodedPayload.length != index) + revert PythErrors.InvalidGovernanceMessage(); } } diff --git a/ethereum/contracts/pyth/PythUpgradable.sol b/ethereum/contracts/pyth/PythUpgradable.sol index c9937f69..6c84733e 100644 --- a/ethereum/contracts/pyth/PythUpgradable.sol +++ b/ethereum/contracts/pyth/PythUpgradable.sol @@ -11,6 +11,7 @@ import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; import "./PythGovernance.sol"; import "./Pyth.sol"; +import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol"; contract PythUpgradable is Initializable, @@ -126,11 +127,10 @@ contract PythUpgradable is _upgradeToAndCallUUPS(payload.newImplementation, new bytes(0), false); // Calling a method using `this.` will cause a contract call that will use - // the new contract. - require( - this.pythUpgradableMagic() == 0x97a6f304, - "the new implementation is not a Pyth contract" - ); + // the new contract. This call will fail if the method does not exists or the magic + // is different. + if (this.pythUpgradableMagic() != 0x97a6f304) + revert PythErrors.InvalidGovernanceMessage(); emit ContractUpgraded(oldImplementation, _getImplementation()); } diff --git a/ethereum/forge-test/GasBenchmark.t.sol b/ethereum/forge-test/GasBenchmark.t.sol index ce088be3..24f5c306 100644 --- a/ethereum/forge-test/GasBenchmark.t.sol +++ b/ethereum/forge-test/GasBenchmark.t.sol @@ -6,6 +6,7 @@ import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import "forge-std/Test.sol"; import "@pythnetwork/pyth-sdk-solidity/IPyth.sol"; +import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol"; import "@pythnetwork/pyth-sdk-solidity/PythStructs.sol"; import "./utils/WormholeTestUtils.t.sol"; import "./utils/PythTestUtils.t.sol"; @@ -139,11 +140,7 @@ contract GasBenchmark is Test, WormholeTestUtils, PythTestUtils { function testBenchmarkUpdatePriceFeedsIfNecessaryNotFresh() public { // Since the price is not advanced, the publishTimes are the same as the // ones in the contract. - vm.expectRevert( - bytes( - "no prices in the submitted batch have fresh prices, so this update will have no effect" - ) - ); + vm.expectRevert(PythErrors.NoFreshUpdate.selector); pyth.updatePriceFeedsIfNecessary{value: cachedPricesUpdateFee}( cachedPricesUpdateData, @@ -183,11 +180,7 @@ contract GasBenchmark is Test, WormholeTestUtils, PythTestUtils { bytes32[] memory ids = new bytes32[](1); ids[0] = priceIds[0]; - vm.expectRevert( - bytes( - "1 or more price feeds are not found in the updateData or they are out of the given time range" - ) - ); + vm.expectRevert(PythErrors.PriceFeedNotFoundWithinRange.selector); pyth.parsePriceFeedUpdates{value: freshPricesUpdateFee}( freshPricesUpdateData, ids, diff --git a/ethereum/forge-test/Pyth.t.sol b/ethereum/forge-test/Pyth.t.sol index b288545b..f1fe76a0 100644 --- a/ethereum/forge-test/Pyth.t.sol +++ b/ethereum/forge-test/Pyth.t.sol @@ -6,6 +6,7 @@ import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import "forge-std/Test.sol"; import "@pythnetwork/pyth-sdk-solidity/IPyth.sol"; +import "@pythnetwork/pyth-sdk-solidity/PythErrors.sol"; import "@pythnetwork/pyth-sdk-solidity/PythStructs.sol"; import "./utils/WormholeTestUtils.t.sol"; import "./utils/PythTestUtils.t.sol"; @@ -364,7 +365,7 @@ contract PythTest is Test, WormholeTestUtils, PythTestUtils, RandTestUtils { // Since attestations are not empty the fee should be at least 1 assertGe(updateFee, 1); - vm.expectRevert(bytes("insufficient paid fee amount")); + vm.expectRevert(PythErrors.InsufficientFee.selector); pyth.parsePriceFeedUpdates{value: updateFee - 1}( updateData, @@ -425,7 +426,7 @@ contract PythTest is Test, WormholeTestUtils, PythTestUtils, RandTestUtils { uint updateFee = pyth.getUpdateFee(updateData); - vm.expectRevert(bytes("invalid data source chain/emitter ID")); + vm.expectRevert(PythErrors.InvalidUpdateDataSource.selector); pyth.parsePriceFeedUpdates{value: updateFee}( updateData, priceIds, @@ -455,7 +456,7 @@ contract PythTest is Test, WormholeTestUtils, PythTestUtils, RandTestUtils { uint updateFee = pyth.getUpdateFee(updateData); - vm.expectRevert(bytes("invalid data source chain/emitter ID")); + vm.expectRevert(PythErrors.InvalidUpdateDataSource.selector); pyth.parsePriceFeedUpdates{value: updateFee}( updateData, priceIds, @@ -480,11 +481,7 @@ contract PythTest is Test, WormholeTestUtils, PythTestUtils, RandTestUtils { bytes32[] memory priceIds = new bytes32[](1); priceIds[0] = bytes32(uint(2)); - vm.expectRevert( - bytes( - "1 or more price feeds are not found in the updateData or they are out of the given time range" - ) - ); + vm.expectRevert(PythErrors.PriceFeedNotFoundWithinRange.selector); pyth.parsePriceFeedUpdates{value: updateFee}( updateData, priceIds, @@ -520,11 +517,7 @@ contract PythTest is Test, WormholeTestUtils, PythTestUtils, RandTestUtils { ); // Request for parse after the time range should revert. - vm.expectRevert( - bytes( - "1 or more price feeds are not found in the updateData or they are out of the given time range" - ) - ); + vm.expectRevert(PythErrors.PriceFeedNotFoundWithinRange.selector); pyth.parsePriceFeedUpdates{value: updateFee}( updateData, priceIds, diff --git a/ethereum/package-lock.json b/ethereum/package-lock.json index 80d98bdf..4f81e47d 100644 --- a/ethereum/package-lock.json +++ b/ethereum/package-lock.json @@ -13,7 +13,7 @@ "@certusone/wormhole-sdk-wasm": "^0.0.1", "@openzeppelin/contracts": "^4.5.0", "@openzeppelin/contracts-upgradeable": "^4.5.2", - "@pythnetwork/pyth-sdk-solidity": "^2.1.0", + "@pythnetwork/pyth-sdk-solidity": "^2.2.0", "@pythnetwork/xc-governance-sdk": "file:../third_party/pyth/xc-governance-sdk-js", "dotenv": "^10.0.0", "elliptic": "^6.5.2", @@ -5642,9 +5642,9 @@ "integrity": "sha1-p3c2C1s5oaLlEG+OhY8v0tBgxXA=" }, "node_modules/@pythnetwork/pyth-sdk-solidity": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/@pythnetwork/pyth-sdk-solidity/-/pyth-sdk-solidity-2.1.0.tgz", - "integrity": "sha512-jHzqw+BHaCOAYwRNCgAUhcbNZrB5f3Arly3PaYN3/Tg7/5RQ95a9FD15XvJB1DB3yymUPIkmLYMur7Sh+e1G4A==" + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/@pythnetwork/pyth-sdk-solidity/-/pyth-sdk-solidity-2.2.0.tgz", + "integrity": "sha512-LsRMmaf9MTflGSymqOJMepFk/3R7DyxMOJfLDB5RDSieyiq+RJ5IYIYnXAFsMrqkjibOtVxARcortHtE9VWwhw==" }, "node_modules/@pythnetwork/xc-governance-sdk": { "resolved": "../third_party/pyth/xc-governance-sdk-js", @@ -45038,9 +45038,9 @@ "integrity": "sha1-p3c2C1s5oaLlEG+OhY8v0tBgxXA=" }, "@pythnetwork/pyth-sdk-solidity": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/@pythnetwork/pyth-sdk-solidity/-/pyth-sdk-solidity-2.1.0.tgz", - "integrity": "sha512-jHzqw+BHaCOAYwRNCgAUhcbNZrB5f3Arly3PaYN3/Tg7/5RQ95a9FD15XvJB1DB3yymUPIkmLYMur7Sh+e1G4A==" + "version": "2.2.0", + "resolved": "https://registry.npmjs.org/@pythnetwork/pyth-sdk-solidity/-/pyth-sdk-solidity-2.2.0.tgz", + "integrity": "sha512-LsRMmaf9MTflGSymqOJMepFk/3R7DyxMOJfLDB5RDSieyiq+RJ5IYIYnXAFsMrqkjibOtVxARcortHtE9VWwhw==" }, "@pythnetwork/xc-governance-sdk": { "version": "file:../third_party/pyth/xc-governance-sdk-js", diff --git a/ethereum/package.json b/ethereum/package.json index de5b7bb5..a79d66c2 100644 --- a/ethereum/package.json +++ b/ethereum/package.json @@ -32,7 +32,7 @@ "@certusone/wormhole-sdk-wasm": "^0.0.1", "@openzeppelin/contracts": "^4.5.0", "@openzeppelin/contracts-upgradeable": "^4.5.2", - "@pythnetwork/pyth-sdk-solidity": "^2.1.0", + "@pythnetwork/pyth-sdk-solidity": "^2.2.0", "@pythnetwork/xc-governance-sdk": "file:../third_party/pyth/xc-governance-sdk-js", "dotenv": "^10.0.0", "elliptic": "^6.5.2", diff --git a/ethereum/test/pyth.js b/ethereum/test/pyth.js index 323df580..d317167b 100644 --- a/ethereum/test/pyth.js +++ b/ethereum/test/pyth.js @@ -39,7 +39,6 @@ contract("Pyth", function () { "0x71f8dcb863d176e2c420ad6610cf687359612b6fb392e0642b0ca6b1f186aa3b"; const notOwnerError = "Ownable: caller is not the owner -- Reason given: Ownable: caller is not the owner."; - const insufficientFeeError = "insufficient paid fee amount"; // Place all atomic operations that are done within migrations here. beforeEach(async function () { @@ -433,9 +432,9 @@ contract("Pyth", function () { assert.equal(feeInWei, 20); // When a smaller fee is payed it reverts - await expectRevert( + await expectRevertCustomError( updatePriceFeeds(this.pythProxy, [rawBatch1, rawBatch2], feeInWei - 1), - insufficientFeeError + "InsufficientFee" ); }); @@ -571,11 +570,11 @@ contract("Pyth", function () { }); it("should fail transaction if a price is not found", async function () { - await expectRevert( + await expectRevertCustomError( this.pythProxy.queryPriceFeed( "0xdeadfeeddeadfeeddeadfeeddeadfeeddeadfeeddeadfeeddeadfeeddeadfeed" ), - "price feed for the given id is not pushed or does not exist" + "PriceFeedNotFound" ); }); @@ -591,10 +590,7 @@ contract("Pyth", function () { for (var i = 1; i <= RAW_BATCH_ATTESTATION_COUNT; i++) { const price_id = "0x" + (255 - (i % 256)).toString(16).padStart(2, "0").repeat(32); - expectRevert( - this.pythProxy.getPrice(price_id), - "no price available which is recent enough" - ); + expectRevertCustomError(this.pythProxy.getPrice(price_id), "StalePrice"); } }); @@ -610,10 +606,7 @@ contract("Pyth", function () { for (var i = 1; i <= RAW_BATCH_ATTESTATION_COUNT; i++) { const price_id = "0x" + (255 - (i % 256)).toString(16).padStart(2, "0").repeat(32); - expectRevert( - this.pythProxy.getPrice(price_id), - "no price available which is recent enough" - ); + expectRevertCustomError(this.pythProxy.getPrice(price_id), "StalePrice"); } }); @@ -643,10 +636,7 @@ contract("Pyth", function () { const price_id = "0x" + (255 - (i % 256)).toString(16).padStart(2, "0").repeat(32); - expectRevert( - this.pythProxy.getPrice(price_id), - "no price available which is recent enough" - ); + expectRevertCustomError(this.pythProxy.getPrice(price_id), "StalePrice"); } // Setting the validity time to 120 seconds @@ -753,9 +743,9 @@ contract("Pyth", function () { 0 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.updatePriceFeeds(["0x" + vm]), - "invalid data source chain/emitter ID" + "InvalidUpdateDataSource" ); }); @@ -779,9 +769,9 @@ contract("Pyth", function () { 1 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaaWrongMagic), - "invalid magic for GovernanceInstruction" + "InvalidGovernanceMessage" ); const wrongModule = Buffer.from(data); @@ -794,9 +784,9 @@ contract("Pyth", function () { 1 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaaWrongModule), - "invalid module for GovernanceInstruction" + "InvalidGovernanceTarget" ); const outOfBoundModule = Buffer.from(data); @@ -828,9 +818,9 @@ contract("Pyth", function () { 1 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaaWrongEmitter), - "VAA is not coming from the governance data source" + "InvalidGovernanceDataSource" ); const vaaWrongChain = await createVAAFromUint8Array( @@ -840,9 +830,9 @@ contract("Pyth", function () { 1 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaaWrongChain), - "VAA is not coming from the governance data source" + "InvalidGovernanceDataSource" ); }); @@ -859,9 +849,9 @@ contract("Pyth", function () { 1 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(wrongChainVaa), - "invalid target chain for this governance instruction" + "InvalidGovernanceTarget" ); const dataForAllChains = new governance.SetValidPeriodInstruction( @@ -908,9 +898,9 @@ contract("Pyth", function () { await this.pythProxy.executeGovernanceInstruction(vaaSeq1), // Replaying shouldn't work - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaaSeq1), - "VAA is older than the last executed governance VAA" + "OldGovernanceMessage" ); const vaaSeq2 = await createVAAFromUint8Array( @@ -922,13 +912,13 @@ contract("Pyth", function () { await this.pythProxy.executeGovernanceInstruction(vaaSeq2), // Replaying shouldn't work - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaaSeq1), - "VAA is older than the last executed governance VAA" + "OldGovernanceMessage" ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaaSeq2), - "VAA is older than the last executed governance VAA" + "OldGovernanceMessage" ); }); @@ -948,9 +938,9 @@ contract("Pyth", function () { 1 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaa), - "upgrade with chain id 0 is not possible" + "InvalidGovernanceTarget" ); }); @@ -1020,9 +1010,9 @@ contract("Pyth", function () { 1 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(claimVaaHexString), - "VAA is not coming from the governance data source" + "InvalidGovernanceDataSource" ); const claimVaa = Buffer.from(claimVaaHexString.substring(2), "hex"); @@ -1055,9 +1045,9 @@ contract("Pyth", function () { expect(newGovernanceDataSource.emitterAddress).equal(newEmitterAddress); // Verifies the data source has changed. - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(vaa), - "VAA is not coming from the governance data source" + "InvalidGovernanceDataSource" ); // Make sure a claim vaa does not get executed @@ -1075,9 +1065,9 @@ contract("Pyth", function () { 2 ); - await expectRevert( + await expectRevertCustomError( this.pythProxy.executeGovernanceInstruction(claimLonelyVaa), - "RequestGovernanceDataSourceTransfer can be only part of AuthorizeGovernanceDataSourceTransfer message" + "InvalidGovernanceMessage" ); // Transfer back the ownership to the old governance data source without increasing @@ -1115,9 +1105,15 @@ contract("Pyth", function () { 2 ); - await expectRevert( - this.pythProxy.executeGovernanceInstruction(transferBackVaaWrong), - "cannot upgrade to an older governance data source" + // This test fails without the hard coded gas limit. + // Without gas limit, it fails on a random place (in wormhole sig verification) which + // is probably because truffle cannot estimate the gas usage correctly. So the gas is + // hard-coded to a high value of 6.7m gas (close to ganache limit). + await expectRevertCustomError( + this.pythProxy.executeGovernanceInstruction(transferBackVaaWrong, { + gas: 6700000, + }), + "OldGovernanceMessage" ); }); @@ -1163,9 +1159,9 @@ contract("Pyth", function () { ); let rawBatch = generateRawBatchAttestation(100, 100, 1337); - await expectRevert( + await expectRevertCustomError( updatePriceFeeds(this.pythProxy, [rawBatch]), - "invalid data source chain/emitter ID" + "InvalidUpdateDataSource" ); await updatePriceFeeds( @@ -1202,9 +1198,9 @@ contract("Pyth", function () { assert.equal(await this.pythProxy.singleUpdateFeeInWei(), "5000"); let rawBatch = generateRawBatchAttestation(100, 100, 1337); - await expectRevert( + await expectRevertCustomError( updatePriceFeeds(this.pythProxy, [rawBatch], 0), - insufficientFeeError + "InsufficientFee" ); await updatePriceFeeds(this.pythProxy, [rawBatch], 5000); @@ -1385,3 +1381,18 @@ function expectEventMultipleTimes(receipt, eventName, args, cnt) { const matches = getNumMatchingEvents(receipt, eventName, args); assert(matches === cnt, `Expected ${cnt} event matches, found ${matches}.`); } + +async function expectRevertCustomError(promise, reason) { + try { + await promise; + expect.fail("Expected promise to throw but it didn't"); + } catch (revert) { + if (reason) { + const reasonId = web3.utils.keccak256(reason + "()").substr(0, 10); + expect( + JSON.stringify(revert), + `Expected custom error ${reason} (${reasonId})` + ).to.include(reasonId); + } + } +}