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

MultiAggregateRateLimiter - validation based contract with OffRamp implementation #916

Merged
merged 26 commits into from
Jun 3, 2024

Conversation

elatoskinas
Copy link
Collaborator

@elatoskinas elatoskinas commented May 23, 2024

Motivation

Re-designs the usage of the aggregate rate limiter, with a focus on the OffRamp. The main motivations are:

  • reduction of contract size in the OffRamp (and in next PR - for the OnRamp) - from 24.79KB to 20.32KB
  • increased flexibility & decoupling for message validations, allowing to perform more than just rate limit checks
  • ability to more easily control opt-in validations, with an easy way to globally disable them

Solution

The solution consists of multiple parts:

  • IMessageValidator: an interface for hook contracts that can be plugged into OffRamps/OnRamps to validate messages. The validation functions revert if the validation criteria are not met
  • MultiAggregateRateLimiter: the AggregateRateLimiter contract, but converted to support multiple chains, with rate-limit specific logic pulled in from the OffRamp. The contract now implements the IMessageValidator interface
  • MultiOffRamp: allows configuring an optional IMessageValidator, which can perform any validation (in this case, we could set it to a MultiAggregateRateLimiter contract)
  • (removed) RateLimiterNoEvents: the RateLimiter library converted to exclude events (since they should now be per-chain). It was not converted to a RateLimiterMultiChain to maintain the simplicity of the library (since it should only act as a token bucket)

Design choices

  • validateIncomingMessage does not batch messages. The reason for this is:
    • To preserve generic chain inputs Any2EVM. Passing a batch of Any2EVM messages requires more complex refactoring since we only have EVM2EVM messages, which only get converted in executeSingleMessage
    • Batching multiple messages would mean that rate limiting could fail all messages, as opposed to treating them as isolated messages and succeeding a large batch (e.g. in the current approach, the rate limit could fail 1 out of 3 messages, but the other 2 would succeed. In batching, this would not be the case)
    • The logic is more in line with the previous releaseOrMint logic, which treats each message as a single isolated unit
    • Note that the main drawback is the gas cost - we are making multiple external calls in this case
  • A single MultiAggregateRateLimiter / IMessageValidator contract should be used by both the OnRamp and the OffRamp, which means that the rate limiting state is shared
    • (this is now separated per-lane) We do not make a distinction of lane src -> dest and dest -> src
  • All IMessageValidator get caught by the validations in the ramps. The reason is simplicity - unexpected failures should not trigger unexpected bubbling reverts, and with this approach we do not need to handle each specific validation error
  • The message validator is expected to be shared by all lanes in a single multi-ramp contract

Out of scope for this PR

  • OnRamp handling and on-ramp specific configurations in MultiAggregateRateLimiter
  • Message batching MultiARL / IMessageValidator (passing an array instead of a single message)
  • Considerations on whether the MultiARL should be per-lane or global
  • (the expected behaviour is to not reset the ARL state on lane re-deployments) Multi-ARL state resets on re-deployment for a lane

Copy link
Contributor

github-actions bot commented May 23, 2024

LCOV of commit 0faa19f during Solidity Foundry #4864

Summary coverage rate:
  lines......: 98.7% (1485 of 1505 lines)
  functions..: 96.5% (303 of 314 functions)
  branches...: 92.7% (643 of 694 branches)

Files changed coverage rate: n/a

@elatoskinas elatoskinas marked this pull request as ready for review May 23, 2024 12:39
@elatoskinas elatoskinas requested review from RensR, matYang, makramkd and a team as code owners May 23, 2024 12:39
@@ -1,75 +1,284 @@
// SPDX-License-Identifier: BUSL-1.1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: would like to move this under a separate folder group (under validators/) - keeping in root for diff

@RensR
Copy link
Collaborator

RensR commented May 24, 2024

A single MultiAggregateRateLimiter / IMessageValidator contract can be used by both the OnRamp and the OffRamp, which means that the rate limiting state is shared
We do not make a distinction of lane src -> dest and dest -> src

I don't follow, would you be combining the outgoing and incoming rate limits? We cannot do that

@RensR
Copy link
Collaborator

RensR commented May 24, 2024

Multi-ARL state resets on re-deployment for a lane

You mean it should reset? I'd argue it doesn't

Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@elatoskinas elatoskinas merged commit 7a97d12 into ccip-develop Jun 3, 2024
76 of 77 checks passed
@elatoskinas elatoskinas deleted the feat/multi-offramp-arl-hook branch June 3, 2024 08:06
elatoskinas added a commit that referenced this pull request Jun 7, 2024
…rt (#939)

## Motivation
Follow up of #916 to integrate non-EVM token support, and convert
`rateLimitedTokens` configuration to be per-chain. This converts the
multi-ARL to be truly multi-lane and untied from EVM2EVM to EVM2Any,
Any2EVM

## Solution
* Add `chainSelector` to rate limit tokens state
* Convert addresses from `bytes` to `bytes32` (for now assuming that we
can fit token addresses in `bytes32` for all non-EVM chains)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants