EVM: NttManagerNoRateLimiting code review rework

README.md

@@ -12,17 +12,15 @@ Wormhole’s Native Token Transfers (NTT) is an open, flexible, and composable f
 
 There are two basic components to NTT:
 
-(1) **Transceiver**: This contract is responsible for sending NTT transfers forwarded through the `NttManager` on the source chain and delivered to a corresponding peer `NttManager` on the recipient chain. Transceivers should follow the `ITransceiver` interface. Transceivers can be defined independently of Wormhole core and can be modified to support any verification backend.
-
-(2) **NttManager**: The NttManager contract is responsible for managing the token and the Transceivers. It also handles the rate-limiting and the message attestation logic. Note that each `NttManager` corresponds to a single token. However, a single `NttManager` can can control multiple transceivers.
+(1) **Transceiver**: This contract is responsible for sending NTT transfers forwarded through the `NttManager` on the source chain and delivered to a corresponding peer `NttManager` on the recipient chain. Transceivers should follow the `ITransceiver` interface. Transceivers can be defined independently of Wormhole core and can be modified to support any verification backend. See [docs/Transceiver.md](./docs/Transceiver.md) for more info.
 
+(2) **NttManager**: The NttManager contract is responsible for managing the token and the Transceivers. It also handles the rate-limiting and the message attestation logic. Note that each `NttManager` corresponds to a single token. However, a single `NttManager` can control multiple transceivers. See [docs/NttManager.md](./docs/NttManager.md) for more info.
 
 <figure>
  <img src="images/ntt_architecture__with_custom_attestation.jpg" alt="NTT Architecture Diagram">
  <figcaption>Figure: NTT Architecture Diagram with Custom Attestations.</figcaption>
 </figure>
 
-
 ## Amount trimming
 
 In the payload, amounts are encoded as unsigned 64 bit integers, and capped at the configured `TRIMMED_DECIMALS` (e.g. 8) decimal value.
@@ -52,5 +50,6 @@ The action identifier specifies the runtime. Currently, these are as follows:
 - 1: evm
 - 2: solana
 
-___
+---
+
 ⚠️ **WARNING:** Ensure that if the `NttManager` on the source chain is configured to be in `LOCKING` mode, the corresponding `NttManager`s on the target chains are configured to be in `BURNING` mode. If not, transfers will NOT go through and user funds may be lost! Proceed with caution!

evm/README.md

@@ -226,9 +226,11 @@ cp WormholeNttConfig.json.sample WormholeNttConfig.json
 
 Configure each network to your liking (including adding/removing networks). We will eventually add the addresses of the deployed contracts to this file. Navigate back to the `evm` directory.
 
-___
+---
+
 ⚠️ **WARNING:** Ensure that if the `NttManager` on the source chain is configured to be in `LOCKING` mode, the corresponding `NttManager`s on the target chains are configured to be in `BURNING` mode. If not, transfers will NOT go through and user funds may be lost! Proceed with caution!
-___
+
+---
 
 Currently the per-chain `inBoundLimit` is set to zero by default. This means all inbound transfers will be queued by the rate limiter. Set this value accordingly.
 
@@ -267,3 +269,9 @@ bash sh/configure_wormhole_ntt.sh -n NETWORK_TYPE -c CHAIN_NAME -k PRIVATE_KEY
 Tokens powered by NTT in **burn** mode require the `burn` method to be present. This method is not present in the standard ERC20 interface, but is found in the `ERC20Burnable` interface.
 
 The `mint` and `setMinter` methods found in the [`INttToken` Interface](src/interfaces/INttToken.sol) are not found in the standard `ERC20` interface.
+
+#### No-Rate-Limiting
+
+Although the rate limiting feature can be disabled during contract instantiation, it still occupies code space in the `NttManager` contract. If you wish to free up code space for custom development, you can instead instantiate the
+`NttManagerNoRateLimiting` contract. This contract is built without the bulk of the rate limiting code. Note that the current immutable checks do not allow modifying the rate-limiting parameters during migration. This means migrating
+from an instance of `NttManager` with rate-limiting enabled to `NttManagerNoRateLimiting` or vice versa is not officially supported.

evm/src/NttManager/NttManager.sol

@@ -223,7 +223,7 @@ contract NttManager is INttManager, RateLimiter, ManagerBase {
 
  address transferRecipient = fromWormholeFormat(nativeTokenTransfer.to);
 
- bool enqueued = _enqueueOrConsumeIndboundRateLimit(
+ bool enqueued = _enqueueOrConsumeInboundRateLimit(
  digest, sourceChainId, nativeTransferAmount, transferRecipient
  );
 
@@ -234,7 +234,7 @@ contract NttManager is INttManager, RateLimiter, ManagerBase {
  _mintOrUnlockToRecipient(digest, transferRecipient, nativeTransferAmount, false);
  }
 
- function _enqueueOrConsumeIndboundRateLimit(
+ function _enqueueOrConsumeInboundRateLimit(
  bytes32 digest,
  uint16 sourceChainId,
  TrimmedAmount nativeTransferAmount,

evm/src/NttManager/NttManagerNoRateLimiting.sol

@@ -3,6 +3,13 @@ pragma solidity >=0.8.8 <0.9.0;
 
 import "./NttManager.sol";
 
+/// @title NttManagerNoRateLimiting
+/// @author Wormhole Project Contributors.
+/// @notice The NttManagerNoRateLimiting contract is an implementation of
+/// NttManager that eliminates most of the rate limiting code to
+/// free up code space.
+///
+/// @dev All of the developer notes from `NttManager` apply here.
 contract NttManagerNoRateLimiting is NttManager {
  constructor(
  address _token,
@@ -95,7 +102,7 @@ contract NttManagerNoRateLimiting is NttManager {
  return false;
  }
 
- function _enqueueOrConsumeIndboundRateLimit(
+ function _enqueueOrConsumeInboundRateLimit(
  bytes32, // digest
  uint16, // sourceChainId
  TrimmedAmount, // nativeTransferAmount

evm/test/NttManagerNoRateLimiting.t.sol

@@ -975,6 +975,48 @@ contract TestNttManagerNoRateLimiting is Test, IRateLimiterEvents {
  assertEq(token.balanceOf(address(user_B)), transferAmount.untrim(token.decimals()) * 2);
  }
 
+ // NOTE: There are additional tests in `Upgrades.t.sol` to verifying upgrading from `NttManager` to `NttManagerNoRateLimiting`.
+
+ function test_canUpgradeFromNoRateLimitingToRateLimitingDisabled() public {
+ // Create a standard manager with rate limiting disabled.
+ DummyToken t = new DummyToken();
+ NttManager implementation =
+ new MockNttManagerContract(address(t), IManagerBase.Mode.LOCKING, chainId, 0, true);
+
+ MockNttManagerContract thisNttManager =
+ MockNttManagerContract(address(new ERC1967Proxy(address(implementation), "")));
+ thisNttManager.initialize();
+
+ thisNttManager.setPeer(chainId2, toWormholeFormat(address(0x1)), 9, type(uint64).max);
+
+ // Upgrade from NttManagerNoRateLimiting to NttManager with rate limiting enabled. This should work.
+ NttManager rateLimitingImplementation =
+ new MockNttManagerNoRateLimitingContract(address(t), IManagerBase.Mode.LOCKING, chainId);
+
+ thisNttManager.upgrade(address(rateLimitingImplementation));
+ }
+
+ function test_cannotUpgradeFromNoRateLimitingToRateLimitingEnaabled() public {
+ // Create a standard manager with rate limiting enabled.
+ DummyToken t = new DummyToken();
+ NttManager implementation = new MockNttManagerContract(
+ address(t), IManagerBase.Mode.LOCKING, chainId, 1 days, false
+ );
+
+ MockNttManagerContract thisNttManager =
+ MockNttManagerContract(address(new ERC1967Proxy(address(implementation), "")));
+ thisNttManager.initialize();
+
+ thisNttManager.setPeer(chainId2, toWormholeFormat(address(0x1)), 9, type(uint64).max);
+
+ // Upgrade from NttManagerNoRateLimiting to NttManager with rate limiting enabled. The immutable check should panic.
+ NttManager rateLimitingImplementation =
+ new MockNttManagerNoRateLimitingContract(address(t), IManagerBase.Mode.LOCKING, chainId);
+
+ vm.expectRevert(); // Reverts with a panic on the assert. So, no way to tell WHY this happened.
+ thisNttManager.upgrade(address(rateLimitingImplementation));
+ }
+
  function test_tokenUpgradedAndDecimalsChanged() public {
  DummyToken dummy1 = new DummyTokenMintAndBurn();
 

evm/test/Upgrades.t.sol

@@ -184,6 +184,8 @@ contract TestUpgrades is Test, IRateLimiterEvents {
  basicFunctionality();
  }
 
+ // NOTE: There are additional tests in `Upgrades.t.sol` to verifying downgrading from `NttManagerNoRateLimiting` to `NttManager`.
+
  function test_cannotUpgradeToNoRateLimitingIfItWasEnabled() public {
  // The default set up has rate limiting enabled. When we attempt to upgrade to no rate limiting, the immutable check should panic.
  NttManager rateLimitingImplementation = new MockNttManagerNoRateLimitingContract(