Skip to content

Commit

Permalink
Misc golfs and fixes (#1402)
Browse files Browse the repository at this point in the history 
Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
  • Loading branch information
@RensR @app-token-issuer-infra-releng @makramkd
2 people authored and makramkd committed Sep 4, 2024
1 parent 8853040 commit f0fc8b4
Show file tree
Hide file tree
Showing 12 changed files with 182 additions and 195 deletions.
276 changes: 138 additions & 138 deletions contracts/gas-snapshots/ccip.gas-snapshot
View file

Large diffs are not rendered by default.

6 changes: 2 additions & 4 deletions contracts/src/v0.8/ccip/FeeQuoter.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,8 +467,6 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
uint256 numberOfTokens = message.tokenAmounts.length;
_validateMessage(destChainConfig, message.data.length, numberOfTokens, message.receiver);

uint64 premiumMultiplierWeiPerEth = s_premiumMultiplierWeiPerEth[message.feeToken];

// The below call asserts that feeToken is a supported token
(uint224 feeTokenPrice, uint224 packedGasPrice) = getTokenAndGasPrices(message.feeToken, destChainSelector);

Expand Down Expand Up @@ -511,7 +509,6 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,

// NOTE: when supporting non-EVM chains, revisit how generic this fee logic can be
// NOTE: revisit parsing non-EVM args

uint256 executionCost = uint112(packedGasPrice)
* (
destChainConfig.destGasOverhead + (message.data.length * destChainConfig.destGasPerPayloadByte) + tokenTransferGas
Expand All @@ -521,7 +518,8 @@ contract FeeQuoter is AuthorizedCallers, IFeeQuoter, ITypeAndVersion, IReceiver,
// Calculate number of fee tokens to charge.
// Total USD fee is in 36 decimals, feeTokenPrice is in 18 decimals USD for 1e18 smallest token denominations.
// Result of the division is the number of smallest token denominations.
return ((premiumFee * premiumMultiplierWeiPerEth) + executionCost + dataAvailabilityCost) / feeTokenPrice;
return ((premiumFee * s_premiumMultiplierWeiPerEth[message.feeToken]) + executionCost + dataAvailabilityCost)
/ feeTokenPrice;
}

/// @notice Sets the fee configuration for a token.
Expand Down
30 changes: 12 additions & 18 deletions contracts/src/v0.8/ccip/capability/CCIPConfig.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol";
import {ICapabilitiesRegistry} from "./interfaces/ICapabilitiesRegistry.sol";

import {OwnerIsCreator} from "../../shared/access/OwnerIsCreator.sol";

import {SortedSetValidationUtil} from "../../shared/util/SortedSetValidationUtil.sol";
import {Internal} from "../libraries/Internal.sol";
import {CCIPConfigTypes} from "./libraries/CCIPConfigTypes.sol";
Expand All @@ -26,7 +25,6 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
/// @param chainSelector The chain selector.
/// @param chainConfig The chain configuration.
event ChainConfigSet(uint64 chainSelector, CCIPConfigTypes.ChainConfig chainConfig);

/// @notice Emitted when a chain's configuration is removed.
/// @param chainSelector The chain selector.
event ChainConfigRemoved(uint64 chainSelector);
Expand Down Expand Up @@ -172,9 +170,8 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
revert OnlyCapabilitiesRegistryCanCall();
}

CCIPConfigTypes.OCR3Config[] memory ocr3Configs = abi.decode(config, (CCIPConfigTypes.OCR3Config[]));
(CCIPConfigTypes.OCR3Config[] memory commitConfigs, CCIPConfigTypes.OCR3Config[] memory execConfigs) =
_groupByPluginType(ocr3Configs);
_groupByPluginType(abi.decode(config, (CCIPConfigTypes.OCR3Config[])));
if (commitConfigs.length > 0) {
_updatePluginConfig(donId, Internal.OCRPluginType.Commit, commitConfigs);
}
Expand Down Expand Up @@ -363,8 +360,8 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
// access in the for loop below.
commitConfigs = new CCIPConfigTypes.OCR3Config[](MAX_OCR3_CONFIGS_PER_PLUGIN);
execConfigs = new CCIPConfigTypes.OCR3Config[](MAX_OCR3_CONFIGS_PER_PLUGIN);
uint256 commitCount;
uint256 execCount;
uint256 commitCount = 0;
uint256 execCount = 0;
for (uint256 i = 0; i < ocr3Configs.length; ++i) {
if (ocr3Configs[i].pluginType == Internal.OCRPluginType.Commit) {
commitConfigs[commitCount] = ocr3Configs[i];
Expand Down Expand Up @@ -418,9 +415,7 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
SortedSetValidationUtil._checkIsValidUniqueSubset(cfg.bootstrapP2PIds, cfg.p2pIds);

// Check that the readers are in the capabilities registry.
for (uint256 i = 0; i < cfg.signers.length; ++i) {
_ensureInRegistry(cfg.p2pIds[i]);
}
_ensureInRegistry(cfg.p2pIds);
}

/// @notice Computes the digest of the provided configuration.
Expand Down Expand Up @@ -486,13 +481,10 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
// Process additions next.
for (uint256 i = 0; i < chainConfigAdds.length; ++i) {
CCIPConfigTypes.ChainConfig memory chainConfig = chainConfigAdds[i].chainConfig;
bytes32[] memory readers = chainConfig.readers;
uint64 chainSelector = chainConfigAdds[i].chainSelector;

// Verify that the provided readers are present in the capabilities registry.
for (uint256 j = 0; j < readers.length; j++) {
_ensureInRegistry(readers[j]);
}
_ensureInRegistry(chainConfig.readers);

// Verify that fChain is positive.
if (chainConfig.fChain == 0) {
Expand All @@ -507,11 +499,13 @@ contract CCIPConfig is ITypeAndVersion, ICapabilityConfiguration, OwnerIsCreator
}

/// @notice Helper function to ensure that a node is in the capabilities registry.
/// @param p2pId The P2P ID of the node to check.
function _ensureInRegistry(bytes32 p2pId) internal view {
ICapabilitiesRegistry.NodeInfo memory node = ICapabilitiesRegistry(i_capabilitiesRegistry).getNode(p2pId);
if (node.p2pId == bytes32("")) {
revert NodeNotInRegistry(p2pId);
/// @param p2pIds The P2P IDs of the node to check.
function _ensureInRegistry(bytes32[] memory p2pIds) internal view {
for (uint256 i = 0; i < p2pIds.length; ++i) {
// TODO add a method that does the validation in the ICapabilitiesRegistry contract
if (ICapabilitiesRegistry(i_capabilitiesRegistry).getNode(p2pIds[i]).p2pId == bytes32("")) {
revert NodeNotInRegistry(p2pIds[i]);
}
}
}
}
15 changes: 6 additions & 9 deletions contracts/src/v0.8/ccip/ocr/MultiOCR3Base.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,7 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {
}

address[] memory transmitters = ocrConfigArgs.transmitters;
// Transmitters are expected to never exceed 255 (since this is bounded by MAX_NUM_ORACLES)
uint8 newTransmittersLength = uint8(transmitters.length);

if (newTransmittersLength > MAX_NUM_ORACLES) revert InvalidConfig(InvalidConfigErrorType.TOO_MANY_TRANSMITTERS);
if (transmitters.length > MAX_NUM_ORACLES) revert InvalidConfig(InvalidConfigErrorType.TOO_MANY_TRANSMITTERS);

_clearOracleRoles(ocrPluginType, ocrConfig.transmitters);

Expand Down Expand Up @@ -198,13 +195,13 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {
/// @param oracleAddresses Oracle addresses to assign roles to.
/// @param role Role to assign.
function _assignOracleRoles(uint8 ocrPluginType, address[] memory oracleAddresses, Role role) internal {
for (uint8 i = 0; i < oracleAddresses.length; ++i) {
for (uint256 i = 0; i < oracleAddresses.length; ++i) {
address oracle = oracleAddresses[i];
if (s_oracles[ocrPluginType][oracle].role != Role.Unset) {
revert InvalidConfig(InvalidConfigErrorType.REPEATED_ORACLE_ADDRESS);
}
if (oracle == address(0)) revert OracleCannotBeZeroAddress();
s_oracles[ocrPluginType][oracle] = Oracle(i, role);
s_oracles[ocrPluginType][oracle] = Oracle(uint8(i), role);
}
}

Expand Down Expand Up @@ -294,7 +291,7 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {
bytes32 rawVs // signatures
) internal view {
// Verify signatures attached to report
bool[MAX_NUM_ORACLES] memory signed;
uint256 signed = 0;

uint256 numberOfSignatures = rs.length;
for (uint256 i; i < numberOfSignatures; ++i) {
Expand All @@ -304,8 +301,8 @@ abstract contract MultiOCR3Base is ITypeAndVersion, OwnerIsCreator {
// never have a signer role.
Oracle memory oracle = s_oracles[ocrPluginType][signer];
if (oracle.role != Role.Signer) revert UnauthorizedSigner();
if (signed[oracle.index]) revert NonUniqueSignatures();
signed[oracle.index] = true;
if (signed & (0x1 << oracle.index) != 0) revert NonUniqueSignatures();
signed |= 0x1 << oracle.index;
}
}

Expand Down
17 changes: 9 additions & 8 deletions contracts/src/v0.8/ccip/offRamp/OffRamp.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ pragma solidity 0.8.24;

import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol";
import {IAny2EVMMessageReceiver} from "../interfaces/IAny2EVMMessageReceiver.sol";

import {IFeeQuoter} from "../interfaces/IFeeQuoter.sol";
import {IMessageInterceptor} from "../interfaces/IMessageInterceptor.sol";
import {INonceManager} from "../interfaces/INonceManager.sol";
Expand Down Expand Up @@ -138,7 +137,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
MerkleRoot[] merkleRoots; // Collection of merkle roots per source chain to commit
}

/// @dev Struct to hold a merkle root for a source chain so that an array of these can be passed in the resetUblessedRoots function.
/// @dev Struct to hold a merkle root for a source chain so that an array of these can be passed in the
/// resetUnblessedRoots function.
struct UnblessedRoot {
uint64 sourceChainSelector; // Remote source chain selector that the Merkle Root is scoped to
bytes32 merkleRoot; // Merkle root of a single remote source chain
Expand Down Expand Up @@ -344,7 +344,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
uint64 sourceChainSelector = report.sourceChainSelector;
_whenNotCursed(sourceChainSelector);

SourceChainConfig storage sourceChainConfig = _getEnabledSourceChainConfig(sourceChainSelector);
bytes memory onRamp = _getEnabledSourceChainConfig(sourceChainSelector).onRamp;

uint256 numMsgs = report.messages.length;
if (numMsgs == 0) revert EmptyReport();
Expand All @@ -365,7 +365,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
// over the same data, which increases gas cost.
// Hashing all of the message fields ensures that the message being executed is correct and not tampered with.
// Including the known OnRamp ensures that the message originates from the correct on ramp version
hashedLeaves[i] = Internal._hash(message, sourceChainConfig.onRamp);
hashedLeaves[i] = Internal._hash(message, onRamp);
}

// SECURITY CRITICAL CHECK
Expand Down Expand Up @@ -445,7 +445,6 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
}

_setExecutionState(sourceChainSelector, message.header.sequenceNumber, Internal.MessageExecutionState.IN_PROGRESS);

(Internal.MessageExecutionState newState, bytes memory returnData) = _trialExecute(message, offchainTokenData);
_setExecutionState(sourceChainSelector, message.header.sequenceNumber, newState);

Expand Down Expand Up @@ -477,6 +476,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
hashedLeaves[i],
newState,
returnData,
// This emit covers not only the execution through the router, but also all of the overhead in executing the
// message. This gives the most accurate representation of the gas used in the execution.
gasStart - gasleft()
);
}
Expand Down Expand Up @@ -586,12 +587,12 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
// Check if the report contains price updates
if (commitReport.priceUpdates.tokenPriceUpdates.length > 0 || commitReport.priceUpdates.gasPriceUpdates.length > 0)
{
uint64 sequenceNumber = uint64(uint256(reportContext[1]));
uint64 ocrSequenceNumber = uint64(uint256(reportContext[1]));

// Check for price staleness based on the epoch and round
if (s_latestPriceSequenceNumber < sequenceNumber) {
if (s_latestPriceSequenceNumber < ocrSequenceNumber) {
// If prices are not stale, update the latest epoch and round
s_latestPriceSequenceNumber = sequenceNumber;
s_latestPriceSequenceNumber = ocrSequenceNumber;
// And update the prices in the fee quoter
IFeeQuoter(s_dynamicConfig.feeQuoter).updatePrices(commitReport.priceUpdates);
} else {
Expand Down
11 changes: 4 additions & 7 deletions contracts/src/v0.8/ccip/onRamp/OnRamp.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
//solhint-disable gas-struct-packing
struct AllowListConfigArgs {
uint64 destChainSelector; // ─────────────╮ Destination chain selector
// │ destChainSelector and allowListEnabled are packed in the same slot
// │ destChainSelector and allowListEnabled are packed in the same slot
bool allowListEnabled; // ────────────────╯ boolean indicator to specify if allowList check is enabled.
address[] addedAllowlistedSenders; // list of senders to be added to the allowedSendersList
address[] removedAllowlistedSenders; // list of senders to be removed from the allowedSendersList
Expand Down Expand Up @@ -208,7 +208,7 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
}),
sender: originalSender,
data: message.data,
extraArgs: message.extraArgs,
extraArgs: convertedExtraArgs,
receiver: message.receiver,
feeToken: message.feeToken,
feeTokenAmount: feeTokenAmount,
Expand All @@ -232,9 +232,6 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
newMessage.tokenAmounts[i].destExecData = destExecDataPerToken[i];
}

// Override extraArgs with latest version
newMessage.extraArgs = convertedExtraArgs;

// Hash only after all fields have been set
newMessage.header.messageId = Internal._hash(
newMessage,
Expand Down Expand Up @@ -425,8 +422,8 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
}
}

for (uint256 k = 0; k < allowListConfigArgs.removedAllowlistedSenders.length; ++k) {
destChainConfig.allowedSendersList.remove(allowListConfigArgs.removedAllowlistedSenders[k]);
for (uint256 j = 0; j < allowListConfigArgs.removedAllowlistedSenders.length; ++j) {
destChainConfig.allowedSendersList.remove(allowListConfigArgs.removedAllowlistedSenders[j]);
}

if (allowListConfigArgs.removedAllowlistedSenders.length > 0) {
Expand Down
Loading

0 comments on commit f0fc8b4

Please sign in to comment.