Skip to content

Commit

Permalink
Remove TODO and rename messageValidator to messageInterceptor (#1438)
Browse files Browse the repository at this point in the history
- Replaces `CCIPConfigTypes.ConfigState` `/// TODO: explain rollbacks?`
by rollbacks comment
- Renames `messageValidator` to `messageInterceptor` in ramps

---------

Co-authored-by: Rens Rooimans <[email protected]>
  • Loading branch information
RayXpub and RensR authored Sep 13, 2024
1 parent f169c88 commit bd417ed
Show file tree
Hide file tree
Showing 12 changed files with 83 additions and 76 deletions.
42 changes: 21 additions & 21 deletions contracts/gas-snapshots/ccip.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -671,15 +671,15 @@ OffRamp_execute:test_UnauthorizedTransmitter_Revert() (gas: 149068)
OffRamp_execute:test_WrongConfigWithSigners_Revert() (gas: 6999426)
OffRamp_execute:test_ZeroReports_Revert() (gas: 17293)
OffRamp_executeSingleMessage:test_MessageSender_Revert() (gas: 18874)
OffRamp_executeSingleMessage:test_NonContractWithTokens_Success() (gas: 248743)
OffRamp_executeSingleMessage:test_NonContractWithTokens_Success() (gas: 248721)
OffRamp_executeSingleMessage:test_NonContract_Success() (gas: 21206)
OffRamp_executeSingleMessage:test_TokenHandlingError_Revert() (gas: 209803)
OffRamp_executeSingleMessage:test_ZeroGasDONExecution_Revert() (gas: 49525)
OffRamp_executeSingleMessage:test_executeSingleMessage_NoTokens_Success() (gas: 49013)
OffRamp_executeSingleMessage:test_executeSingleMessage_WithFailingValidationNoRouterCall_Revert() (gas: 218730)
OffRamp_executeSingleMessage:test_executeSingleMessage_WithFailingValidation_Revert() (gas: 85512)
OffRamp_executeSingleMessage:test_executeSingleMessage_WithTokens_Success() (gas: 280659)
OffRamp_executeSingleMessage:test_executeSingleMessage_WithValidation_Success() (gas: 91830)
OffRamp_executeSingleMessage:test_executeSingleMessage_WithVInterception_Success() (gas: 91829)
OffRamp_executeSingleReport:test_DisabledSourceChain_Revert() (gas: 28730)
OffRamp_executeSingleReport:test_EmptyReport_Revert() (gas: 22018)
OffRamp_executeSingleReport:test_InvalidSourcePoolAddress_Success() (gas: 503897)
Expand Down Expand Up @@ -741,9 +741,9 @@ OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_Success() (gas: 177447)
OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_WithGasOverride_Success() (gas: 179427)
OffRamp_releaseOrMintTokens:test_releaseOrMintTokens_destDenominatedDecimals_Success() (gas: 190636)
OffRamp_setDynamicConfig:test_FeeQuoterZeroAddress_Revert() (gas: 11201)
OffRamp_setDynamicConfig:test_NonOwner_Revert() (gas: 13811)
OffRamp_setDynamicConfig:test_SetDynamicConfigWithValidator_Success() (gas: 46216)
OffRamp_setDynamicConfig:test_SetDynamicConfig_Success() (gas: 24191)
OffRamp_setDynamicConfig:test_NonOwner_Revert() (gas: 13789)
OffRamp_setDynamicConfig:test_SetDynamicConfigWithInterceptor_Success() (gas: 46192)
OffRamp_setDynamicConfig:test_SetDynamicConfig_Success() (gas: 24234)
OffRamp_trialExecute:test_RateLimitError_Success() (gas: 226586)
OffRamp_trialExecute:test_TokenHandlingErrorIsCaught_Success() (gas: 235208)
OffRamp_trialExecute:test_TokenPoolIsNotAContract_Success() (gas: 314371)
Expand All @@ -759,29 +759,29 @@ OnRamp_constructor:test_Constructor_InvalidConfigNonceManagerEqAddressZero_Rever
OnRamp_constructor:test_Constructor_InvalidConfigRMNProxyEqAddressZero_Revert() (gas: 97783)
OnRamp_constructor:test_Constructor_InvalidConfigTokenAdminRegistryEqAddressZero_Revert() (gas: 92815)
OnRamp_constructor:test_Constructor_Success() (gas: 2817599)
OnRamp_forwardFromRouter:test_ForwardFromRouterExtraArgsV2AllowOutOfOrderTrue_Success() (gas: 114939)
OnRamp_forwardFromRouter:test_ForwardFromRouterExtraArgsV2AllowOutOfOrderTrue_Success() (gas: 114917)
OnRamp_forwardFromRouter:test_ForwardFromRouterExtraArgsV2_Success() (gas: 145710)
OnRamp_forwardFromRouter:test_ForwardFromRouterSuccessCustomExtraArgs() (gas: 145313)
OnRamp_forwardFromRouter:test_ForwardFromRouterSuccessCustomExtraArgs() (gas: 145291)
OnRamp_forwardFromRouter:test_ForwardFromRouterSuccessEmptyExtraArgs() (gas: 143538)
OnRamp_forwardFromRouter:test_ForwardFromRouterSuccessLegacyExtraArgs() (gas: 145538)
OnRamp_forwardFromRouter:test_ForwardFromRouter_Success() (gas: 144917)
OnRamp_forwardFromRouter:test_ForwardFromRouter_Success_ConfigurableSourceRouter() (gas: 140249)
OnRamp_forwardFromRouter:test_ForwardFromRouterSuccessLegacyExtraArgs() (gas: 145516)
OnRamp_forwardFromRouter:test_ForwardFromRouter_Success() (gas: 144895)
OnRamp_forwardFromRouter:test_ForwardFromRouter_Success_ConfigurableSourceRouter() (gas: 140292)
OnRamp_forwardFromRouter:test_InvalidExtraArgsTag_Revert() (gas: 28824)
OnRamp_forwardFromRouter:test_MessageValidationError_Revert() (gas: 138938)
OnRamp_forwardFromRouter:test_MessageInterceptionError_Revert() (gas: 138916)
OnRamp_forwardFromRouter:test_MesssageFeeTooHigh_Revert() (gas: 26924)
OnRamp_forwardFromRouter:test_MultiCannotSendZeroTokens_Revert() (gas: 74139)
OnRamp_forwardFromRouter:test_OriginalSender_Revert() (gas: 12951)
OnRamp_forwardFromRouter:test_Paused_Revert() (gas: 37338)
OnRamp_forwardFromRouter:test_Permissions_Revert() (gas: 18284)
OnRamp_forwardFromRouter:test_ShouldIncrementNonceOnlyOnOrdered_Success() (gas: 184562)
OnRamp_forwardFromRouter:test_MultiCannotSendZeroTokens_Revert() (gas: 74162)
OnRamp_forwardFromRouter:test_OriginalSender_Revert() (gas: 12995)
OnRamp_forwardFromRouter:test_Paused_Revert() (gas: 37361)
OnRamp_forwardFromRouter:test_Permissions_Revert() (gas: 18307)
OnRamp_forwardFromRouter:test_ShouldIncrementNonceOnlyOnOrdered_Success() (gas: 184585)
OnRamp_forwardFromRouter:test_ShouldIncrementSeqNumAndNonce_Success() (gas: 210880)
OnRamp_forwardFromRouter:test_ShouldStoreLinkFees() (gas: 124838)
OnRamp_forwardFromRouter:test_ShouldStoreLinkFees() (gas: 124861)
OnRamp_forwardFromRouter:test_ShouldStoreNonLinkFees() (gas: 141462)
OnRamp_forwardFromRouter:test_SourceTokenDataTooLarge_Revert() (gas: 3769244)
OnRamp_forwardFromRouter:test_SourceTokenDataTooLarge_Revert() (gas: 3769222)
OnRamp_forwardFromRouter:test_UnAllowedOriginalSender_Revert() (gas: 18714)
OnRamp_forwardFromRouter:test_UnsupportedToken_Revert() (gas: 110919)
OnRamp_forwardFromRouter:test_forwardFromRouter_UnsupportedToken_Revert() (gas: 76235)
OnRamp_forwardFromRouter:test_forwardFromRouter_WithValidation_Success() (gas: 281240)
OnRamp_forwardFromRouter:test_UnsupportedToken_Revert() (gas: 110897)
OnRamp_forwardFromRouter:test_forwardFromRouter_UnsupportedToken_Revert() (gas: 76213)
OnRamp_forwardFromRouter:test_forwardFromRouter_WithInterception_Success() (gas: 281260)
OnRamp_getFee:test_EmptyMessage_Success() (gas: 97734)
OnRamp_getFee:test_EnforceOutOfOrder_Revert() (gas: 64215)
OnRamp_getFee:test_GetFeeOfZeroForTokenMessage_Success() (gas: 85428)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@ library CCIPConfigTypes {
/// The only valid transition from "Init" is to the "Running" state - this is the first ever configuration.
/// The only valid transition from "Running" is to the "Staging" state - this is a blue/green proposal.
/// The only valid transition from "Staging" is back to the "Running" state - this is a promotion.
/// TODO: explain rollbacks?
/// In order to rollback a configuration, we must therefore do the following:
/// - Suppose that we have a correct configuration in the "Running" state (V1).
/// - We propose a new configuration and transition to the "Staging" state (V2).
/// - V2 turns out to be buggy
/// - In the same transaction, we must:
/// - Promote V2
/// - Re-propose V1
/// - Promote V1
enum ConfigState {
Init,
Running,
Expand Down
8 changes: 4 additions & 4 deletions contracts/src/v0.8/ccip/offRamp/OffRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
struct DynamicConfig {
address feeQuoter; // ──────────────────────────────╮ FeeQuoter address on the local chain
uint32 permissionLessExecutionThresholdSeconds; //──╯ Waiting time before manual execution is enabled
address messageValidator; // Optional message validator to validate incoming messages (zero address = no validator)
address messageInterceptor; // Optional message interceptor to validate incoming messages (zero address = no interceptor)
}

/// @dev Report that is committed by the observing DON at the committing phase
Expand Down Expand Up @@ -565,9 +565,9 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base {
destTokenAmounts: destTokenAmounts
});

address messageValidator = s_dynamicConfig.messageValidator;
if (messageValidator != address(0)) {
try IMessageInterceptor(messageValidator).onInboundMessage(any2EvmMessage) {}
address messageInterceptor = s_dynamicConfig.messageInterceptor;
if (messageInterceptor != address(0)) {
try IMessageInterceptor(messageInterceptor).onInboundMessage(any2EvmMessage) {}
catch (bytes memory err) {
revert IMessageInterceptor.MessageValidationError(err);
}
Expand Down
8 changes: 4 additions & 4 deletions contracts/src/v0.8/ccip/onRamp/OnRamp.sol
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {
// solhint-disable-next-line gas-struct-packing
struct DynamicConfig {
address feeQuoter; // FeeQuoter address
address messageValidator; // Optional message validator to validate outbound messages (zero address = no validator)
address messageInterceptor; // Optional message interceptor to validate outbound messages (zero address = no interceptor)
address feeAggregator; // Fee aggregator address
address allowListAdmin; // authorized admin to add or remove allowed senders
}
Expand Down Expand Up @@ -179,9 +179,9 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator {

{
// scoped to reduce stack usage
address messageValidator = s_dynamicConfig.messageValidator;
if (messageValidator != address(0)) {
IMessageInterceptor(messageValidator).onOutboundMessage(destChainSelector, message);
address messageInterceptor = s_dynamicConfig.messageInterceptor;
if (messageInterceptor != address(0)) {
IMessageInterceptor(messageInterceptor).onOutboundMessage(destChainSelector, message);
}
}

Expand Down
22 changes: 11 additions & 11 deletions contracts/src/v0.8/ccip/test/offRamp/OffRamp.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,9 @@ contract OffRamp_setDynamicConfig is OffRampSetup {
_assertSameConfig(dynamicConfig, newConfig);
}

function test_SetDynamicConfigWithValidator_Success() public {
function test_SetDynamicConfigWithInterceptor_Success() public {
OffRamp.DynamicConfig memory dynamicConfig = _generateDynamicOffRampConfig(address(s_feeQuoter));
dynamicConfig.messageValidator = address(s_inboundMessageValidator);
dynamicConfig.messageInterceptor = address(s_inboundMessageInterceptor);

vm.expectEmit();
emit OffRamp.DynamicConfigSet(dynamicConfig);
Expand Down Expand Up @@ -1038,10 +1038,10 @@ contract OffRamp_executeSingleMessage is OffRampSetup {
s_offRamp.executeSingleMessage(message, offchainTokenData, new uint32[](0));
}

function test_executeSingleMessage_WithValidation_Success() public {
function test_executeSingleMessage_WithVInterception_Success() public {
vm.stopPrank();
vm.startPrank(OWNER);
_enableInboundMessageValidator();
_enableInboundMessageInterceptor();
vm.startPrank(address(s_offRamp));
Internal.Any2EVMRampMessage memory message =
_generateAny2EVMMessageNoTokens(SOURCE_CHAIN_SELECTOR_1, ON_RAMP_ADDRESS_1, 1);
Expand Down Expand Up @@ -1108,11 +1108,11 @@ contract OffRamp_executeSingleMessage is OffRampSetup {
function test_executeSingleMessage_WithFailingValidation_Revert() public {
vm.stopPrank();
vm.startPrank(OWNER);
_enableInboundMessageValidator();
_enableInboundMessageInterceptor();
vm.startPrank(address(s_offRamp));
Internal.Any2EVMRampMessage memory message =
_generateAny2EVMMessageNoTokens(SOURCE_CHAIN_SELECTOR_1, ON_RAMP_ADDRESS_1, 1);
s_inboundMessageValidator.setMessageIdValidationState(message.header.messageId, true);
s_inboundMessageInterceptor.setMessageIdValidationState(message.header.messageId, true);
vm.expectRevert(
abi.encodeWithSelector(
IMessageInterceptor.MessageValidationError.selector,
Expand All @@ -1125,7 +1125,7 @@ contract OffRamp_executeSingleMessage is OffRampSetup {
function test_executeSingleMessage_WithFailingValidationNoRouterCall_Revert() public {
vm.stopPrank();
vm.startPrank(OWNER);
_enableInboundMessageValidator();
_enableInboundMessageInterceptor();
vm.startPrank(address(s_offRamp));

Internal.Any2EVMRampMessage memory message =
Expand All @@ -1136,7 +1136,7 @@ contract OffRamp_executeSingleMessage is OffRampSetup {
message.receiver = address(newReceiver);
message.header.messageId = Internal._hash(message, ON_RAMP_ADDRESS_1);

s_inboundMessageValidator.setMessageIdValidationState(message.header.messageId, true);
s_inboundMessageInterceptor.setMessageIdValidationState(message.header.messageId, true);
vm.expectRevert(
abi.encodeWithSelector(
IMessageInterceptor.MessageValidationError.selector,
Expand Down Expand Up @@ -2088,7 +2088,7 @@ contract OffRamp_execute is OffRampSetup {
}

function test_MultipleReportsWithPartialValidationFailures_Success() public {
_enableInboundMessageValidator();
_enableInboundMessageInterceptor();

Internal.Any2EVMRampMessage[] memory messages1 = new Internal.Any2EVMRampMessage[](2);
Internal.Any2EVMRampMessage[] memory messages2 = new Internal.Any2EVMRampMessage[](1);
Expand All @@ -2101,8 +2101,8 @@ contract OffRamp_execute is OffRampSetup {
reports[0] = _generateReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages1);
reports[1] = _generateReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages2);

s_inboundMessageValidator.setMessageIdValidationState(messages1[0].header.messageId, true);
s_inboundMessageValidator.setMessageIdValidationState(messages2[0].header.messageId, true);
s_inboundMessageInterceptor.setMessageIdValidationState(messages1[0].header.messageId, true);
s_inboundMessageInterceptor.setMessageIdValidationState(messages2[0].header.messageId, true);

vm.expectEmit();
emit MultiOCR3Base.Transmitted(
Expand Down
12 changes: 6 additions & 6 deletions contracts/src/v0.8/ccip/test/offRamp/OffRampSetup.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract OffRampSetup is FeeQuoterSetup, MultiOCR3BaseSetup {
MaybeRevertingBurnMintTokenPool internal s_maybeRevertingPool;

OffRampHelper internal s_offRamp;
MessageInterceptorHelper internal s_inboundMessageValidator;
MessageInterceptorHelper internal s_inboundMessageInterceptor;
NonceManager internal s_inboundNonceManager;
RMN internal s_realRMN;
address internal s_sourceTokenPool = makeAddr("sourceTokenPool");
Expand All @@ -60,7 +60,7 @@ contract OffRampSetup is FeeQuoterSetup, MultiOCR3BaseSetup {
FeeQuoterSetup.setUp();
MultiOCR3BaseSetup.setUp();

s_inboundMessageValidator = new MessageInterceptorHelper();
s_inboundMessageInterceptor = new MessageInterceptorHelper();
s_receiver = new MaybeRevertMessageReceiver(false);
s_secondary_receiver = new MaybeRevertMessageReceiver(false);
s_reverting_receiver = new MaybeRevertMessageReceiver(true);
Expand Down Expand Up @@ -227,7 +227,7 @@ contract OffRampSetup is FeeQuoterSetup, MultiOCR3BaseSetup {
return OffRamp.DynamicConfig({
permissionLessExecutionThresholdSeconds: PERMISSION_LESS_EXECUTION_THRESHOLD_SECONDS,
feeQuoter: feeQuoter,
messageValidator: address(0)
messageInterceptor: address(0)
});
}

Expand Down Expand Up @@ -382,7 +382,7 @@ contract OffRampSetup is FeeQuoterSetup, MultiOCR3BaseSetup {

function _assertSameConfig(OffRamp.DynamicConfig memory a, OffRamp.DynamicConfig memory b) public pure {
assertEq(a.permissionLessExecutionThresholdSeconds, b.permissionLessExecutionThresholdSeconds);
assertEq(a.messageValidator, b.messageValidator);
assertEq(a.messageInterceptor, b.messageInterceptor);
assertEq(a.feeQuoter, b.feeQuoter);
}

Expand Down Expand Up @@ -412,9 +412,9 @@ contract OffRampSetup is FeeQuoterSetup, MultiOCR3BaseSetup {
return sourceTokenData;
}

function _enableInboundMessageValidator() internal {
function _enableInboundMessageInterceptor() internal {
OffRamp.DynamicConfig memory dynamicConfig = s_offRamp.getDynamicConfig();
dynamicConfig.messageValidator = address(s_inboundMessageValidator);
dynamicConfig.messageInterceptor = address(s_inboundMessageInterceptor);
s_offRamp.setDynamicConfig(dynamicConfig);
}

Expand Down
20 changes: 10 additions & 10 deletions contracts/src/v0.8/ccip/test/onRamp/OnRamp.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -363,8 +363,8 @@ contract OnRamp_forwardFromRouter is OnRampSetup {
);
}

function test_forwardFromRouter_WithValidation_Success() public {
_enableOutboundMessageValidator();
function test_forwardFromRouter_WithInterception_Success() public {
_enableOutboundMessageInterceptor();

Client.EVM2AnyMessage memory message = _generateEmptyMessage();
message.extraArgs = Client._argsToBytes(Client.EVMExtraArgsV1({gasLimit: GAS_LIMIT * 2}));
Expand All @@ -373,7 +373,7 @@ contract OnRamp_forwardFromRouter is OnRampSetup {
message.tokenAmounts[0].amount = 1e18;
message.tokenAmounts[0].token = s_sourceTokens[0];
IERC20(s_sourceFeeToken).transferFrom(OWNER, address(s_onRamp), feeAmount);
s_outboundMessageValidator.setMessageIdValidationState(keccak256(abi.encode(message)), false);
s_outboundmessageInterceptor.setMessageIdValidationState(keccak256(abi.encode(message)), false);

vm.expectEmit();
emit OnRamp.CCIPMessageSent(DEST_CHAIN_SELECTOR, _messageToEvent(message, 1, 1, feeAmount, OWNER));
Expand Down Expand Up @@ -420,8 +420,8 @@ contract OnRamp_forwardFromRouter is OnRampSetup {
s_onRamp.forwardFromRouter(DEST_CHAIN_SELECTOR, _generateEmptyMessage(), 0, STRANGER);
}

function test_MessageValidationError_Revert() public {
_enableOutboundMessageValidator();
function test_MessageInterceptionError_Revert() public {
_enableOutboundMessageInterceptor();

Client.EVM2AnyMessage memory message = _generateEmptyMessage();
message.extraArgs = Client._argsToBytes(Client.EVMExtraArgsV1({gasLimit: GAS_LIMIT * 2}));
Expand All @@ -430,7 +430,7 @@ contract OnRamp_forwardFromRouter is OnRampSetup {
message.tokenAmounts[0].amount = 1e18;
message.tokenAmounts[0].token = s_sourceTokens[0];
IERC20(s_sourceFeeToken).transferFrom(OWNER, address(s_onRamp), feeAmount);
s_outboundMessageValidator.setMessageIdValidationState(keccak256(abi.encode(message)), true);
s_outboundmessageInterceptor.setMessageIdValidationState(keccak256(abi.encode(message)), true);

vm.expectRevert(
abi.encodeWithSelector(IMessageInterceptor.MessageValidationError.selector, bytes("Invalid message"))
Expand Down Expand Up @@ -671,7 +671,7 @@ contract OnRamp_setDynamicConfig is OnRampSetup {
OnRamp.StaticConfig memory staticConfig = s_onRamp.getStaticConfig();
OnRamp.DynamicConfig memory newConfig = OnRamp.DynamicConfig({
feeQuoter: address(23423),
messageValidator: makeAddr("messageValidator"),
messageInterceptor: makeAddr("messageInterceptor"),
feeAggregator: FEE_AGGREGATOR,
allowListAdmin: address(0)
});
Expand All @@ -691,7 +691,7 @@ contract OnRamp_setDynamicConfig is OnRampSetup {
OnRamp.DynamicConfig memory newConfig = OnRamp.DynamicConfig({
feeQuoter: address(0),
feeAggregator: FEE_AGGREGATOR,
messageValidator: makeAddr("messageValidator"),
messageInterceptor: makeAddr("messageInterceptor"),
allowListAdmin: address(0)
});

Expand All @@ -702,7 +702,7 @@ contract OnRamp_setDynamicConfig is OnRampSetup {
function test_SetConfigInvalidConfig_Revert() public {
OnRamp.DynamicConfig memory newConfig = OnRamp.DynamicConfig({
feeQuoter: address(23423),
messageValidator: address(0),
messageInterceptor: address(0),
feeAggregator: FEE_AGGREGATOR,
allowListAdmin: address(0)
});
Expand All @@ -716,7 +716,7 @@ contract OnRamp_setDynamicConfig is OnRampSetup {
function test_SetConfigInvalidConfigFeeAggregatorEqAddressZero_Revert() public {
OnRamp.DynamicConfig memory newConfig = OnRamp.DynamicConfig({
feeQuoter: address(23423),
messageValidator: address(0),
messageInterceptor: address(0),
feeAggregator: address(0),
allowListAdmin: address(0)
});
Expand Down
Loading

0 comments on commit bd417ed

Please sign in to comment.