From ab78e5d8d0a26bcbe413509f15155efe7359276d Mon Sep 17 00:00:00 2001 From: rymnc <43716372+rymnc@users.noreply.github.com> Date: Thu, 30 Mar 2023 17:30:54 +0530 Subject: [PATCH] feat: custom errors --- contracts/Rln.sol | 84 +++++++++++++++++++++++++---------------------- docs/index.md | 78 +++++++++++++++++++++++++++++++++++++++++++ test/RLN.t.sol | 69 +++++++++++++++++++++++++++----------- test/rln.test.ts | 4 +-- 4 files changed, 173 insertions(+), 62 deletions(-) diff --git a/contracts/Rln.sol b/contracts/Rln.sol index 2c1ab80..2ec11ab 100644 --- a/contracts/Rln.sol +++ b/contracts/Rln.sol @@ -4,6 +4,34 @@ pragma solidity 0.8.15; import {IPoseidonHasher} from "./PoseidonHasher.sol"; +/// Invalid deposit amount +/// @param required The required deposit amount +/// @param provided The provided deposit amount +error InsufficientDeposit(uint256 required, uint256 provided); + +/// Provided Batch is empty +error EmptyBatch(); + +/// Batch is full, during batch operations +error FullBatch(); + +/// Member is already registered +error DuplicateIdCommitment(); + +/// Batch size mismatch, when the length of secrets and receivers are not equal +/// @param givenSecretsLen The length of the secrets array +/// @param givenReceiversLen The length of the receivers array +error MismatchedBatchSize(uint256 givenSecretsLen, uint256 givenReceiversLen); + +/// Invalid withdrawal address, when the receiver is the contract itself or 0x0 +error InvalidWithdrawalAddress(address to); + +/// Member is not registered +error MemberNotRegistered(uint256 idCommitment); + +/// Member has no stake +error MemberHasNoStake(uint256 idCommitment); + contract RLN { /// @notice The deposit amount required to register as a member uint256 public immutable MEMBERSHIP_DEPOSIT; @@ -49,10 +77,8 @@ contract RLN { /// Allows a user to register as a member /// @param idCommitment The idCommitment of the member function register(uint256 idCommitment) external payable { - require( - msg.value == MEMBERSHIP_DEPOSIT, - "RLN, register: membership deposit is not satisfied" - ); + if (msg.value != MEMBERSHIP_DEPOSIT) + revert InsufficientDeposit(MEMBERSHIP_DEPOSIT, msg.value); _register(idCommitment, msg.value); } @@ -60,15 +86,11 @@ contract RLN { /// @param idCommitments array of idCommitments function registerBatch(uint256[] calldata idCommitments) external payable { uint256 idCommitmentlen = idCommitments.length; - require(idCommitmentlen > 0, "RLN, registerBatch: batch size zero"); - require( - idCommitmentIndex + idCommitmentlen <= SET_SIZE, - "RLN, registerBatch: set is full" - ); - require( - msg.value == MEMBERSHIP_DEPOSIT * idCommitmentlen, - "RLN, registerBatch: membership deposit is not satisfied" - ); + if (idCommitmentlen == 0) revert EmptyBatch(); + if (idCommitmentIndex + idCommitmentlen >= SET_SIZE) revert FullBatch(); + uint256 requiredDeposit = MEMBERSHIP_DEPOSIT * idCommitmentlen; + if (msg.value != requiredDeposit) + revert InsufficientDeposit(requiredDeposit, msg.value); for (uint256 i = 0; i < idCommitmentlen; i++) { _register(idCommitments[i], msg.value / idCommitmentlen); } @@ -78,11 +100,8 @@ contract RLN { /// @param idCommitment The idCommitment of the member /// @param stake The amount of eth staked by the member function _register(uint256 idCommitment, uint256 stake) internal { - require( - !members[idCommitment], - "RLN, _register: member already registered" - ); - require(idCommitmentIndex < SET_SIZE, "RLN, register: set is full"); + if (members[idCommitment]) revert DuplicateIdCommitment(); + if (idCommitmentIndex >= SET_SIZE) revert FullBatch(); members[idCommitment] = true; stakedAmounts[idCommitment] = stake; @@ -99,11 +118,9 @@ contract RLN { address payable[] calldata receivers ) external { uint256 batchSize = secrets.length; - require(batchSize != 0, "RLN, withdrawBatch: batch size zero"); - require( - batchSize == receivers.length, - "RLN, withdrawBatch: batch size mismatch receivers" - ); + if (batchSize == 0) revert EmptyBatch(); + if (batchSize != receivers.length) + revert MismatchedBatchSize(secrets.length, receivers.length); for (uint256 i = 0; i < batchSize; i++) { _withdraw(secrets[i], receivers[i]); } @@ -120,26 +137,15 @@ contract RLN { /// @param secret The idSecretHash of the member /// @param receiver The address to receive the funds function _withdraw(uint256 secret, address payable receiver) internal { - require( - receiver != address(0), - "RLN, _withdraw: empty receiver address" - ); - - require( - receiver != address(this), - "RLN, _withdraw: cannot withdraw to RLN" - ); + if (receiver == address(this) || receiver == address(0)) + revert InvalidWithdrawalAddress(receiver); // derive idCommitment uint256 idCommitment = hash(secret); // check if member is registered - require(members[idCommitment], "RLN, _withdraw: member not registered"); - - // check if member has stake - require( - stakedAmounts[idCommitment] != 0, - "RLN, _withdraw: member has no stake" - ); + if (!members[idCommitment]) revert MemberNotRegistered(idCommitment); + if (stakedAmounts[idCommitment] == 0) + revert MemberHasNoStake(idCommitment); uint256 amountToTransfer = stakedAmounts[idCommitment]; diff --git a/docs/index.md b/docs/index.md index c1c6ae8..aa200b2 100644 --- a/docs/index.md +++ b/docs/index.md @@ -836,6 +836,84 @@ Hashes the input using the Poseidon hash function, n = 2, second input is the co function _hash(uint256 input) internal pure returns (uint256 result) ``` +## InsufficientDeposit + +```solidity +error InsufficientDeposit(uint256 required, uint256 provided) +``` + +Invalid deposit amount + +### Parameters + +| Name | Type | Description | +| -------- | ------- | --------------------------- | +| required | uint256 | The required deposit amount | +| provided | uint256 | The provided deposit amount | + +## EmptyBatch + +```solidity +error EmptyBatch() +``` + +Provided Batch is empty + +## FullBatch + +```solidity +error FullBatch() +``` + +Batch is full, during batch operations + +## DuplicateIdCommitment + +```solidity +error DuplicateIdCommitment() +``` + +Member is already registered + +## MismatchedBatchSize + +```solidity +error MismatchedBatchSize(uint256 givenSecretsLen, uint256 givenReceiversLen) +``` + +Batch size mismatch, when the length of secrets and receivers are not equal + +### Parameters + +| Name | Type | Description | +| ----------------- | ------- | --------------------------------- | +| givenSecretsLen | uint256 | The length of the secrets array | +| givenReceiversLen | uint256 | The length of the receivers array | + +## InvalidWithdrawalAddress + +```solidity +error InvalidWithdrawalAddress(address to) +``` + +Invalid withdrawal address, when the receiver is the contract itself or 0x0 + +## MemberNotRegistered + +```solidity +error MemberNotRegistered(uint256 idCommitment) +``` + +Member is not registered + +## MemberHasNoStake + +```solidity +error MemberHasNoStake(uint256 idCommitment) +``` + +Member has no stake + ## RLN ### MEMBERSHIP_DEPOSIT diff --git a/test/RLN.t.sol b/test/RLN.t.sol index 237412b..fe9fcce 100644 --- a/test/RLN.t.sol +++ b/test/RLN.t.sol @@ -75,18 +75,22 @@ contract RLNTest is Test { rln.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); assertEq(rln.stakedAmounts(idCommitment), MEMBERSHIP_DEPOSIT); assertEq(rln.members(idCommitment), true); - // TODO: use custom errors instead of revert strings - vm.expectRevert(bytes("RLN, _register: member already registered")); + vm.expectRevert(DuplicateIdCommitment.selector); rln.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); } function test__InvalidRegistration__InsufficientDeposit( uint256 idCommitment ) public { + uint256 badDepositAmount = MEMBERSHIP_DEPOSIT - 1; vm.expectRevert( - bytes("RLN, register: membership deposit is not satisfied") + abi.encodeWithSelector( + InsufficientDeposit.selector, + MEMBERSHIP_DEPOSIT, + badDepositAmount + ) ); - rln.register{value: MEMBERSHIP_DEPOSIT - 1}(idCommitment); + rln.register{value: badDepositAmount}(idCommitment); } function test__InvalidRegistration__FullSet( @@ -103,7 +107,7 @@ contract RLNTest is Test { tempRln.register{value: MEMBERSHIP_DEPOSIT}(idCommitmentSeed + i); } assertEq(tempRln.idCommitmentIndex(), 4); - vm.expectRevert(bytes("RLN, register: set is full")); + vm.expectRevert(FullBatch.selector); tempRln.register{value: MEMBERSHIP_DEPOSIT}(idCommitmentSeed + setSize); } @@ -135,13 +139,13 @@ contract RLNTest is Test { assertEq(tempRln.idCommitmentIndex(), 4); uint256[] memory idCommitments = new uint256[](1); idCommitments[0] = idCommitmentSeed + setSize; - vm.expectRevert(bytes("RLN, registerBatch: set is full")); + vm.expectRevert(FullBatch.selector); tempRln.registerBatch{value: MEMBERSHIP_DEPOSIT}(idCommitments); } function test__InvalidBatchRegistration__EmptyBatch() public { uint256[] memory idCommitments = new uint256[](0); - vm.expectRevert(bytes("RLN, registerBatch: batch size zero")); + vm.expectRevert(EmptyBatch.selector); rln.registerBatch{value: MEMBERSHIP_DEPOSIT}(idCommitments); } @@ -150,12 +154,17 @@ contract RLNTest is Test { ) public { vm.assume(isUniqueArray(idCommitments) && idCommitments.length > 0); uint256 idCommitmentlen = idCommitments.length; + uint256 goodDepositAmount = MEMBERSHIP_DEPOSIT * idCommitmentlen; + uint256 badDepositAmount = goodDepositAmount - 1; + vm.expectRevert( - bytes("RLN, registerBatch: membership deposit is not satisfied") - ); - rln.registerBatch{value: MEMBERSHIP_DEPOSIT * idCommitmentlen - 1}( - idCommitments + abi.encodeWithSelector( + InsufficientDeposit.selector, + goodDepositAmount, + badDepositAmount + ) ); + rln.registerBatch{value: badDepositAmount}(idCommitments); } function test__ValidWithdraw( @@ -184,7 +193,12 @@ contract RLNTest is Test { uint256 idCommitment = poseidon.hash(idSecretHash); rln.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); assertEq(rln.stakedAmounts(idCommitment), MEMBERSHIP_DEPOSIT); - vm.expectRevert(bytes("RLN, _withdraw: empty receiver address")); + vm.expectRevert( + abi.encodeWithSelector( + InvalidWithdrawalAddress.selector, + address(0) + ) + ); rln.withdraw(idSecretHash, payable(address(0))); } @@ -193,15 +207,23 @@ contract RLNTest is Test { uint256 idCommitment = poseidon.hash(idSecretHash); rln.register{value: MEMBERSHIP_DEPOSIT}(idCommitment); assertEq(rln.stakedAmounts(idCommitment), MEMBERSHIP_DEPOSIT); - vm.expectRevert(bytes("RLN, _withdraw: cannot withdraw to RLN")); + vm.expectRevert( + abi.encodeWithSelector( + InvalidWithdrawalAddress.selector, + address(rln) + ) + ); rln.withdraw(idSecretHash, payable(address(rln))); } function test__InvalidWithdraw__InvalidIdCommitment( - uint256 idCommitment + uint256 idSecretHash ) public { - vm.expectRevert(bytes("RLN, _withdraw: member not registered")); - rln.withdraw(idCommitment, payable(address(this))); + uint256 idCommitment = poseidon.hash(idSecretHash); + vm.expectRevert( + abi.encodeWithSelector(MemberNotRegistered.selector, idCommitment) + ); + rln.withdraw(idSecretHash, payable(address(this))); } // this shouldn't be possible, but just in case @@ -230,7 +252,9 @@ contract RLNTest is Test { .depth(0) .checked_write(true); - vm.expectRevert(bytes("RLN, _withdraw: member has no stake")); + vm.expectRevert( + abi.encodeWithSelector(MemberHasNoStake.selector, idCommitment) + ); rln.withdraw(idSecretHash, to); } @@ -274,7 +298,7 @@ contract RLNTest is Test { function test__InvalidBatchWithdraw__EmptyBatch() public { uint256[] memory idSecretHashes = new uint256[](0); address payable[] memory to = new address payable[](0); - vm.expectRevert(bytes("RLN, withdrawBatch: batch size zero")); + vm.expectRevert(EmptyBatch.selector); rln.withdrawBatch(idSecretHashes, to); } @@ -287,12 +311,17 @@ contract RLNTest is Test { vm.assume(isUniqueArray(idSecretHashes) && idSecretHashes.length > 0); vm.assume(to != address(0)); + uint256 numberOfReceivers = idSecretHashes.length + 1; vm.expectRevert( - bytes("RLN, withdrawBatch: batch size mismatch receivers") + abi.encodeWithSelector( + MismatchedBatchSize.selector, + idSecretHashes.length, + numberOfReceivers + ) ); rln.withdrawBatch( idSecretHashes, - repeatElementIntoArray(idSecretHashes.length + 1, to) + repeatElementIntoArray(numberOfReceivers, to) ); } } diff --git a/test/rln.test.ts b/test/rln.test.ts index 8c5e298..f7b882d 100644 --- a/test/rln.test.ts +++ b/test/rln.test.ts @@ -82,8 +82,6 @@ describe("RLN", () => { value: price, }); - await expect(registerTx2).to.be.revertedWith( - "RLN, _register: member already registered" - ); + await expect(registerTx2).to.be.revertedWith("DuplicateIdCommitment()"); }); });