Implement Rate Limiter Contract - #851
Conversation
| } | ||
|
|
||
| // Private function to update the rate limits based on the security fund and risk period | ||
| function _updateRateLimits() private { |
There was a problem hiding this comment.
Looks correct, but choice of representation here and in MIP is funny. I think the MIP should express this more clearly, i.e., use
| // Deploy and initialize the RateLimiter contract | ||
| rateLimiter = new RateLimiter(); | ||
| uint256 riskPeriod = 24 * 60 * 60; // 24 hours in seconds | ||
| uint256 securityFund = 5 ether; |
There was a problem hiding this comment.
Probably want to be clear about MOVE token units here and in runbooks.
| // Expect the second transfer to exceed the rate limit and revert | ||
| vm.expectRevert("RATE_LIMIT_EXCEEDED"); | ||
| atomicBridgeCounterpartyMOVE.lockBridgeTransfer( | ||
| initiator, |
There was a problem hiding this comment.
- I'd like to see tests demonstrating both the L1->L2 direction and the L2->L1 direction.
- For user experience, L1->L2 should be able to revert on
initiateBridgeTransfer, no? - In general, why don't we use the
bridgeTransferIdreturned by initiate?
|
@0xmovses is there a good way to test this e2e? Or, should we trust in the forge ~sim and reserve the rest for our time working on operational bits? |
|
@l-monninger e2e will happen in another PR after #690 goes in. |
|
I know this is implemented for the Bilbao Model, but ideally we don't have to report the insurance fund balance and it can instead be queried on-chain. Even if it's just an account, as long as we know the account we can call |
|
@l-monninger sounds good. |
| address owner, | ||
| uint256 _timeLockDuration | ||
| ) public initializer { | ||
| if (_atomicBridgeInitiator == address(0)) revert ZeroAddress(); |
There was a problem hiding this comment.
This is input validation and should be written as:
require(_AtomicBridgeInitiator != address(0), ZeroAddress());| uint256 _timeLockDuration | ||
| ) public initializer { | ||
| if (_atomicBridgeInitiator == address(0)) revert ZeroAddress(); | ||
| if (_rateLimiter == address(0)) revert ZeroAddress(); |
There was a problem hiding this comment.
Input validation:
require (_rateLimiter != address(0), ZeroAddress());| } | ||
|
|
||
| function setRateLimiter(address _rateLimiter) external onlyOwner { | ||
| if (_rateLimiter == address(0)) revert ZeroAddress(); |
There was a problem hiding this comment.
Change to:
require (_rateLimiter != address(0), ZeroAddress());| revert ZeroAddress(); | ||
| } | ||
| if (_moveToken == address(0)) revert ZeroAddress(); | ||
| if (_rateLimiter == address(0)) revert ZeroAddress(); |
There was a problem hiding this comment.
Input validation:
require (_rateLimiter != address(0), ZeroAddress());3 participants
Summary
For a full spec please read MIP-56
Changelog
RateLimiter.solRateLimiterinto Initiator and Counterparty contractsTesting
Adds the solidity unit test
testRateLimitExceededfor both Initiator and Counterparty Contractscd protocol-units/bridge/contracts && forge testOutstanding issues
Integration tests with the relayer should be added in a separate PR, more solidity unit tests could be added to test out the RateLimiter.sol