Skip to content

Commit

Permalink
Start writing tests for SolExtra args and rework a lot of stuff to ac…
Browse files Browse the repository at this point in the history
…complish this
  • Loading branch information
jhweintraub committed Dec 26, 2024
1 parent dd6d04e commit 6c3e296
Show file tree
Hide file tree
Showing 7 changed files with 239 additions and 34 deletions.
123 changes: 98 additions & 25 deletions contracts/src/v0.8/ccip/FeeQuoter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {EnumerableSet} from "../vendor/openzeppelin-solidity/v5.0.2/contracts/ut
/// The authorized callers in the contract represent the fee price updaters.
contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver, KeystoneFeedsPermissionHandler {
using EnumerableSet for EnumerableSet.AddressSet;
using EnumerableSet for EnumerableSet.Bytes32Set;
using USDPriceWith18Decimals for uint224;
using KeystoneFeedDefaultMetadataLib for bytes;

Expand All @@ -44,7 +45,10 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
error MessageTooLarge(uint256 maxSize, uint256 actualSize);
error UnsupportedNumberOfTokens(uint256 numberOfTokens, uint256 maxNumberOfTokensPerMsg);
error InvalidFeeRange(uint256 minFeeUSDCents, uint256 maxFeeUSDCents);
error SolAddressCannotBeWritable(bytes SolAddress);
error SolAddressCannotBeWritable(bytes32 SolAddress);
error CannotUpdateChainFamilySelector(bytes4 chainFamilySelector);
error InvalidChainFamilySelector(bytes4 chainFamilySelector);
error SolExtraArgsAccountsCannotBeZero();

event FeeTokenAdded(address indexed feeToken);
event FeeTokenRemoved(address indexed feeToken);
Expand All @@ -58,6 +62,7 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
event PremiumMultiplierWeiPerEthUpdated(address indexed token, uint64 premiumMultiplierWeiPerEth);
event DestChainConfigUpdated(uint64 indexed destChainSelector, DestChainConfig destChainConfig);
event DestChainAdded(uint64 indexed destChainSelector, DestChainConfig destChainConfig);
event ChainFamilySelectorModified(bytes4 chainFamilySelector, bool isAdded);

/// @dev Contains token price configuration used in both the keystone price updates and the price feed fallback logic.
struct TokenPriceFeedConfig {
Expand Down Expand Up @@ -206,6 +211,10 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
/// @dev The amount of time a token price can be stale before it is considered invalid.
uint32 private immutable i_tokenPriceStalenessThreshold;

/// @dev the Set of all valid chain family selectors as bytes32. Bytes4Set is not supported so we
/// expand to bytes32 for storage.
EnumerableSet.Bytes32Set internal s_validchainFamilySelectors;

constructor(
StaticConfig memory staticConfig,
address[] memory priceUpdaters,
Expand All @@ -226,6 +235,11 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
i_maxFeeJuelsPerMsg = staticConfig.maxFeeJuelsPerMsg;
i_tokenPriceStalenessThreshold = staticConfig.tokenPriceStalenessThreshold;

// These two chain family selectors are known to be supported at deployment so they should be
// added automatically.
s_validchainFamilySelectors.add(bytes32(Internal.CHAIN_FAMILY_SELECTOR_EVM));
s_validchainFamilySelectors.add(bytes32(Internal.CHAIN_FAMILY_SELECTOR_SOL));

_applyFeeTokensUpdates(new address[](0), feeTokens);
_updateTokenPriceFeeds(tokenPriceFeeds);
_applyDestChainConfigUpdates(destChainConfigArgs);
Expand Down Expand Up @@ -849,14 +863,12 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
/// @param chainFamilySelector Tag to identify the target family.
/// @param destAddress Dest address to validate.
/// @dev precondition - assumes the family tag is correct and validated.
/// @dev Since Solana addresses are parsed as bytes32, and no other form of validation occurs, no explicit
/// call is needed to a library function for address validation.
function _validateDestFamilyAddress(bytes4 chainFamilySelector, bytes memory destAddress) internal pure {
if (chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_EVM) {
Internal._validateEVMAddress(destAddress);
}

if (chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_SOL) {
Internal._validateSolAddress(destAddress);
}
}

function _parseGasLimitFromExtraArgBytes(
Expand All @@ -867,26 +879,31 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
return _parseEVMExtraArgsFromBytes(extraArgs, destChainConfig).gasLimit;
} else if (destChainConfig.chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_SOL) {
// If extra args are empty, generate default values.
return _parseSolExtraArgsFromBytes(extraArgs, destChainConfig).computeUnits;
return _parseSolExtraArgsFromBytes(extraArgs, destChainConfig, 0).computeUnits;
}
}

/// @notice Parse and validate the Solana specific Extra Args Bytes.
/// TODO: Finish
/// @param messageLengthBytes The length of the arbitrary message data. If this is non-zero, then an extraArgs
/// MUST be provided otherwise an error will be returned, due to requirements in how Solana accounts operate.
function _parseSolExtraArgsFromBytes(
bytes calldata extraArgs,
DestChainConfig memory destChainConfig
DestChainConfig memory destChainConfig,
uint256 messageLengthBytes
) internal pure returns (Client.SolExtraArgsV1 memory) {
Client.SolExtraArgsV1 memory SolExtraArgs =
_parseUnvalidatedSolExtraArgsFromBytes(extraArgs, destChainConfig.defaultTxGasLimit);

Client.SolExtraArgsV1 memory SolExtraArgs = _parseUnvalidatedSolExtraArgsFromBytes(extraArgs, destChainConfig.defaultTxGasLimit);
if (messageLengthBytes != 0 && SolExtraArgs.accounts.length == 0) revert SolExtraArgsAccountsCannotBeZero();

// Check that compute units is within the allowed range
// Check that compute units is within the allowed range.
if (SolExtraArgs.computeUnits > uint256(destChainConfig.maxPerMsgGasLimit)) revert MessageGasLimitTooHigh();

// The Program name being invoked cannotb
if (SolExtraArgs.accounts[0].isWritable) revert SolAddressCannotBeWritable(SolExtraArgs.accounts[0].pubKey);

// Check that every other account provided is a valid Sol Account
for (uint256 i = 0; i < SolExtraArgs.accounts.length; ++i) {
Internal._validateSolAddress(SolExtraArgs.accounts[i].pubKey);
// The Program name being invoked cannot be writable. If no accounts are provided as extra args, the account
// length check is skipped to prevent an index-out-of-bounds error.
if (SolExtraArgs.accounts.length != 0 && SolExtraArgs.accounts[0].isWritable) {
revert SolAddressCannotBeWritable(SolExtraArgs.accounts[0].pubKey);
}

return SolExtraArgs;
Expand All @@ -898,7 +915,7 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
) internal pure returns (Client.SolExtraArgsV1 memory) {
if (extraArgs.length == 0) {
return Client.SolExtraArgsV1({
computeUnits: uint32(defaultTxGasLimit), //TODO: Potentially unsafe cast
computeUnits: uint32(defaultTxGasLimit), //TODO: Fix Potentially unsafe cast
accounts: new Client.SolanaAccountMeta[](0)
});
}
Expand Down Expand Up @@ -984,7 +1001,10 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,

/// @inheritdoc IFeeQuoter
/// @dev precondition - onRampTokenTransfers and sourceTokenAmounts lengths must be equal.
/// @param message The arbitrary bytes to be processed. Solana address requirements require that if an arbitrary message
/// exists, then extraArgs must also be checked for a valid recipient account.
function processMessageArgs(
bytes calldata message,
uint64 destChainSelector,
address feeToken,
uint256 feeTokenAmount,
Expand All @@ -1001,6 +1021,13 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
bytes[] memory destExecDataPerToken
)
{

// Prevents a stack too deep error on messageData
{
bytes memory messageData = message;
(convertedExtraArgs, isOutOfOrderExecution) = _processChainFamilySelector(destChainSelector, messageData, extraArgs);
}

// Convert feeToken to link if not already in link.
if (feeToken == i_linkToken) {
msgFeeJuels = feeTokenAmount;
Expand All @@ -1010,16 +1037,41 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,

if (msgFeeJuels > i_maxFeeJuelsPerMsg) revert MessageFeeTooHigh(msgFeeJuels, i_maxFeeJuelsPerMsg);

uint64 defaultTxGasLimit = s_destChainConfigs[destChainSelector].defaultTxGasLimit;

// NOTE: Only EVM chains are supported for now, additional validation logic will be added when supporting other
// chain families to parse non-EVM args.
// Since the message is called after getFee, which will already validate the params, no validation is necessary.
Client.EVMExtraArgsV2 memory parsedExtraArgs = _parseUnvalidatedEVMExtraArgsFromBytes(extraArgs, defaultTxGasLimit);
isOutOfOrderExecution = parsedExtraArgs.allowOutOfOrderExecution;
destExecDataPerToken = _processPoolReturnData(destChainSelector, onRampTokenTransfers, sourceTokenAmounts);

return (msgFeeJuels, isOutOfOrderExecution, Client._argsToBytes(parsedExtraArgs), destExecDataPerToken);
return (msgFeeJuels, isOutOfOrderExecution, convertedExtraArgs, destExecDataPerToken);
}

/// @notice Parses the extra Args based on the chain family selector. Isolated into a separate function
/// as it was the only way to prevent a stack too deep error, and makes future chain family additions easier.
function _processChainFamilySelector(
uint64 destChainSelector,
bytes memory message,
bytes calldata extraArgs
) internal view returns (
bytes memory convertedExtraArgs,
bool isOutOfOrderExecution
) {
DestChainConfig memory destChainConfig = s_destChainConfigs[destChainSelector];
if (destChainConfig.chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_EVM) {
// Since the message is called after getFee, which will already validate the params, no validation is necessary.
Client.EVMExtraArgsV2 memory parsedExtraArgs =
_parseUnvalidatedEVMExtraArgsFromBytes(extraArgs, destChainConfig.defaultTxGasLimit);

convertedExtraArgs = Client._argsToBytes(parsedExtraArgs);

isOutOfOrderExecution = parsedExtraArgs.allowOutOfOrderExecution;

} else if (destChainConfig.chainFamilySelector == Internal.CHAIN_FAMILY_SELECTOR_SOL) {
Client.SolExtraArgsV1 memory parsedExtraArgs = _parseSolExtraArgsFromBytes(extraArgs, destChainConfig, message.length);

convertedExtraArgs = Client._solArgsToBytes(parsedExtraArgs);

// On Solana OOO execution is enabled for all messages.
isOutOfOrderExecution = true;
} else {
revert InvalidChainFamilySelector(destChainConfig.chainFamilySelector);
}
}

/// @notice Validates pool return data.
Expand Down Expand Up @@ -1094,10 +1146,11 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
uint64 destChainSelector = destChainConfigArgs[i].destChainSelector;
DestChainConfig memory destChainConfig = destChainConfigArg.destChainConfig;

// destChainSelector must be non-zero, defaultTxGasLimit must be set, and must be less than maxPerMsgGasLimit.
// destChainSelector must be non-zero, defaultTxGasLimit must be set, must be less than maxPerMsgGasLimit, and the chain family selector must be valid.
if (
destChainSelector == 0 || destChainConfig.defaultTxGasLimit == 0
|| destChainConfig.defaultTxGasLimit > destChainConfig.maxPerMsgGasLimit
|| !s_validchainFamilySelectors.contains(bytes32(destChainConfig.chainFamilySelector))
) {
revert InvalidDestChainConfig(destChainSelector);
}
Expand All @@ -1114,6 +1167,26 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
}
}

/// @notice Updates the system to accept a new chain family selector.
/// @dev The validity of a chain family selector affects the ability to add new chains in the applyDestChainConfigUpdates function.
/// @param removes Chain family selectors to remove.
/// @param adds Chain family selectors to add.
function updateChainFamilySelectors(bytes4[] memory removes, bytes4[] memory adds) external onlyOwner {
for (uint256 i = 0; i < removes.length; ++i) {
if (!s_validchainFamilySelectors.remove(bytes32(removes[i]))) {
revert CannotUpdateChainFamilySelector(removes[i]);
}
emit ChainFamilySelectorModified(removes[i], false);
}

for (uint256 i = 0; i < adds.length; ++i) {
if (!s_validchainFamilySelectors.add(bytes32(adds[i]))) {
revert CannotUpdateChainFamilySelector(adds[i]);
}
emit ChainFamilySelectorModified(adds[i], true);
}
}

/// @notice Returns the static FeeQuoter config.
/// @dev RMN depends on this function, if updated, please notify the RMN maintainers.
/// @return staticConfig The static configuration.
Expand Down
2 changes: 2 additions & 0 deletions contracts/src/v0.8/ccip/interfaces/IFeeQuoter.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ interface IFeeQuoter is IPriceRegistry {

/// @notice Converts the extraArgs to the latest version and returns the converted message fee in juels.
/// @notice Validates pool return data.
/// @param message The message to process, necessary when sending messages to Solana to check against extraArgs validity.
/// @param destChainSelector destination chain selector to process, must be a configured valid chain.
/// @param feeToken token address used to pay for message fees, must be a configured valid fee token.
/// @param feeTokenAmount Fee token amount.
Expand All @@ -28,6 +29,7 @@ interface IFeeQuoter is IPriceRegistry {
/// @return convertedExtraArgs extra args converted to the latest family-specific args version.
/// @return destExecDataPerToken Destination chain execution data.
function processMessageArgs(
bytes calldata message,
uint64 destChainSelector,
address feeToken,
uint256 feeTokenAmount,
Expand Down
8 changes: 7 additions & 1 deletion contracts/src/v0.8/ccip/libraries/Client.sol
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ library Client {
}

struct SolanaAccountMeta {
bytes pubKey;
bytes32 pubKey;
bool isWritable;
}

Expand All @@ -69,4 +69,10 @@ library Client {
) internal pure returns (bytes memory bts) {
return abi.encodeWithSelector(EVM_EXTRA_ARGS_V2_TAG, extraArgs);
}

function _solArgsToBytes(
SolExtraArgsV1 memory extraArgs
) internal pure returns (bytes memory bts) {
return abi.encodeWithSelector(SOL_EXTRA_EXTRA_ARGS_V1_TAG, extraArgs);
}
}
9 changes: 8 additions & 1 deletion contracts/src/v0.8/ccip/onRamp/OnRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -221,13 +221,20 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, Ownable2StepMsgSender

// Convert message fee to juels and retrieve converted args.
// Validate pool return data after it is populated (view function - no state changes).
// Using newMessage.data prevents a stack too deep error.
bool isOutOfOrderExecution;
bytes memory convertedExtraArgs;
bytes[] memory destExecDataPerToken;
(newMessage.feeValueJuels, isOutOfOrderExecution, convertedExtraArgs, destExecDataPerToken) = IFeeQuoter(
s_dynamicConfig.feeQuoter
).processMessageArgs(
destChainSelector, message.feeToken, feeTokenAmount, message.extraArgs, newMessage.tokenAmounts, tokenAmounts
newMessage.data,
destChainSelector,
message.feeToken,
feeTokenAmount,
message.extraArgs,
newMessage.tokenAmounts,
tokenAmounts
);

newMessage.header.nonce = isOutOfOrderExecution
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// SPDX-License-Identifier: BUSL-1.1
pragma solidity ^0.8.24;

import {FeeQuoter} from "../../FeeQuoter.sol";
import {Client} from "../../libraries/Client.sol";

import {Internal} from "../../libraries/Internal.sol";
import {FeeQuoterSetup} from "./FeeQuoterSetup.t.sol";

contract FeeQuoter_parseSolExtraArgsFromBytes is FeeQuoterSetup {
FeeQuoter.DestChainConfig private s_destChainConfig;

/// @dev a Valid pubkey is one that is 32 bytes long, and that's it since no other validation can be performed
/// within the constraints of the EVM.
bytes32 internal constant VALID_SOL_PUBKEY = keccak256("valid_sol_pubkey");

function setUp() public virtual override {
super.setUp();
s_destChainConfig = _generateFeeQuoterDestChainConfigArgs()[0].destChainConfig;
s_destChainConfig.chainFamilySelector = Internal.CHAIN_FAMILY_SELECTOR_SOL;
}

function test_SolExtraArgsV1() public view {
Client.SolanaAccountMeta[] memory solAccounts = new Client.SolanaAccountMeta[](1);
solAccounts[0] = Client.SolanaAccountMeta({pubKey: VALID_SOL_PUBKEY, isWritable: false});

Client.SolExtraArgsV1 memory inputArgs = Client.SolExtraArgsV1({computeUnits: GAS_LIMIT, accounts: solAccounts});

bytes memory inputExtraArgs = Client._solArgsToBytes(inputArgs);
uint256 messageDataLengthBytes = 32;

Client.SolExtraArgsV1 memory expectedOutputArgs =
Client.SolExtraArgsV1({computeUnits: GAS_LIMIT, accounts: solAccounts});

vm.assertEq(
abi.encode(s_feeQuoter.parseSOLExtraArgsFromBytes(inputExtraArgs, s_destChainConfig, messageDataLengthBytes)),
abi.encode(expectedOutputArgs)
);
}

function test_SolExtraArgsDefault() public view {
uint256 messageDataLengthBytes = 0;

Client.SolExtraArgsV1 memory expectedOutputArgs = Client.SolExtraArgsV1({
computeUnits: s_destChainConfig.defaultTxGasLimit,
accounts: new Client.SolanaAccountMeta[](0)
});

vm.assertEq(
abi.encode(s_feeQuoter.parseSOLExtraArgsFromBytes("", s_destChainConfig, messageDataLengthBytes)), abi.encode(expectedOutputArgs)
);
}

// Reverts

function test_SolExtraArgsV1_RevertWhen_SolAddressCannotBeWritable() public {}

function test_SolExtraArgsV1_RevertWhen_MessageGasLimitTooHigh() public {
Client.SolExtraArgsV1 memory inputArgs = Client.SolExtraArgsV1({
computeUnits: s_destChainConfig.maxPerMsgGasLimit + 1,
accounts: new Client.SolanaAccountMeta[](0)
});

bytes memory inputExtraArgs = Client._solArgsToBytes(inputArgs);

// The validity of the extra Args depends on any arbitrary data being sent
// in the message so we must simulate that here.
uint256 messageDataLengthBytes = 0;

vm.expectRevert(FeeQuoter.MessageGasLimitTooHigh.selector);
s_feeQuoter.parseSOLExtraArgsFromBytes(inputExtraArgs, s_destChainConfig, messageDataLengthBytes);
}

// function test_RevertWhen_SolExtraArgsEnforceOutOfOrder() public {
// Client.EVMExtraArgsV2 memory inputArgs =
// Client.EVMExtraArgsV2({gasLimit: GAS_LIMIT, allowOutOfOrderExecution: false});
// bytes memory inputExtraArgs = Client._argsToBytes(inputArgs);
// s_destChainConfig.enforceOutOfOrder = true;

// vm.expectRevert(FeeQuoter.ExtraArgOutOfOrderExecutionMustBeTrue.selector);
// s_feeQuoter.parseEVMExtraArgsFromBytes(inputExtraArgs, s_destChainConfig);
// }

// function test_RevertWhen_SolExtraArgsGasLimitTooHigh() public {
// Client.EVMExtraArgsV2 memory inputArgs =
// Client.EVMExtraArgsV2({gasLimit: s_destChainConfig.maxPerMsgGasLimit + 1, allowOutOfOrderExecution: true});
// bytes memory inputExtraArgs = Client._argsToBytes(inputArgs);

// vm.expectRevert(FeeQuoter.MessageGasLimitTooHigh.selector);
// s_feeQuoter.parseEVMExtraArgsFromBytes(inputExtraArgs, s_destChainConfig);
// }
}
Loading

0 comments on commit 6c3e296

Please sign in to comment.