Revert custom errors to require statements #59

minghinmatthewlam · 2023-10-16T21:37:54Z

Why this should be merged

We want to move back from custom errors to require statements because certain toolings are not compatible well with custom errors yet, for example snow trace, or when forge tests fail they're show a hash that needs to be decoded with the abi to be readable. This will cause an increase in contract size and deployment costs but the tradeoff is for easier debugging and integration with existing tools.

How this works

Removes the custom errors in all contracts
Replace custom error checks by inverting if statements and using require statements in the form "{contract}: {error message}"
Updates solhint config to have maximum reason string of 60, and turns off for custom errors
Add scripts/local/lint.sh script
Removes redundant teleporter messenger sender checks for functions called by receiveTeleporterMessage

How this was tested

updating unit tests
ci

BEFORE gas report:

| src/Teleporter/TeleporterMessenger.sol:TeleporterMessenger contract |                 |        |        |        |         |
|---------------------------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost                                                     | Deployment Size |        |        |        |         |
| 2174481                                                             | 10854           |        |        |        |         |
| Function Name                                                       | min             | avg    | median | max    | # calls |
| addFeeAmount                                                        | 3486            | 5545   | 4070   | 9507   | 7       |
| checkIsAllowedRelayer                                               | 950             | 1121   | 950    | 1643   | 5       |
| checkRelayerRewardAmount                                            | 764             | 764    | 764    | 764    | 8       |
| getFeeInfo                                                          | 851             | 1651   | 851    | 4851   | 5       |
| getMessageHash                                                      | 643             | 928    | 643    | 2643   | 7       |
| getNextMessageID                                                    | 660             | 1993   | 2660   | 2660   | 3       |
| getReceiptAtIndex                                                   | 903             | 1038   | 903    | 1616   | 21      |
| getReceiptQueueSize                                                 | 730             | 3896   | 4730   | 4730   | 24      |
| getRelayerRewardAddress                                             | 629             | 754    | 629    | 2629   | 16      |
| messageReceived                                                     | 673             | 839    | 673    | 2673   | 12      |
| receiveCrossChainMessage                                            | 609             | 87978  | 101991 | 178925 | 51      |
| redeemRelayerRewards                                                | 1946            | 2230   | 2008   | 2736   | 3       |
| retryMessageExecution                                               | 3748            | 16726  | 9318   | 57861  | 11      |
| retryReceipts                                                       | 8634            | 37784  | 40410  | 64310  | 3       |
| retrySendCrossChainMessage                                          | 5440            | 7799   | 7753   | 10206  | 3       |
| sendCrossChainMessage                                               | 34540           | 101038 | 111478 | 137275 | 32      |

AFTER gas report:

| src/Teleporter/TeleporterMessenger.sol:TeleporterMessenger contract |                 |        |        |        |         |
|---------------------------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost                                                     | Deployment Size |        |        |        |         |
| 2410954                                                             | 12035           |        |        |        |         |
| Function Name                                                       | min             | avg    | median | max    | # calls |
| addFeeAmount                                                        | 3553            | 5606   | 4142   | 9497   | 7       |
| checkIsAllowedRelayer                                               | 950             | 1121   | 950    | 1643   | 5       |
| checkRelayerRewardAmount                                            | 764             | 764    | 764    | 764    | 8       |
| getFeeInfo                                                          | 851             | 1651   | 851    | 4851   | 5       |
| getMessageHash                                                      | 643             | 928    | 643    | 2643   | 7       |
| getNextMessageID                                                    | 660             | 1993   | 2660   | 2660   | 3       |
| getReceiptAtIndex                                                   | 903             | 1045   | 903    | 1616   | 20      |
| getReceiptQueueSize                                                 | 730             | 3860   | 4730   | 4730   | 23      |
| getRelayerRewardAddress                                             | 629             | 754    | 629    | 2629   | 16      |
| messageReceived                                                     | 673             | 839    | 673    | 2673   | 12      |
| receiveCrossChainMessage                                            | 609             | 87993  | 101991 | 178925 | 51      |
| redeemRelayerRewards                                                | 1946            | 2262   | 2016   | 2826   | 3       |
| retryMessageExecution                                               | 3859            | 16787  | 9380   | 57861  | 11      |
| retryReceipts                                                       | 8724            | 37814  | 40410  | 64310  | 3       |
| retrySendCrossChainMessage                                          | 5551            | 7873   | 7864   | 10206  | 3       |
| sendCrossChainMessage                                               | 34651           | 101042 | 111478 | 137275 | 32      |

bernard-avalabs

LGTM

contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol

bernard-avalabs

There are two msg.sender checks that were removed that I didn't see earlier. Just want to confirm that they are no longer necessary.

contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol

michaelkaplan13 · 2023-10-18T20:30:58Z

contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol

-                revert InsufficientTotalAmount(totalAmount, primaryFeeAmount + secondaryFeeAmount);
-            }
+            require(
+                totalAmount >= primaryFeeAmount + secondaryFeeAmount,


I think this should be > not >=

is this to avoid us calling encodeMintBridgeTokensData with 0 bridge amount? Should we add a check in mint that the amount != 0 if this is the case?

The check in mint would be in the transaction receiving the Teleporter message, rather than the one sending it here. The idea being that the amount bridged excluding fees must be non-zero.

contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol

contracts/src/CrossChainApplications/ERC20Bridge/tests/ERC20BridgeTests.t.sol

contracts/src/Teleporter/TeleporterMessenger.sol

scripts/local/lint.sh

Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>

michaelkaplan13

LGTM!

cam-schultz

A couple of minor comments, but overall LGTM

contracts/src/Teleporter/TeleporterMessenger.sol

contracts/src/Teleporter/tests/TeleporterMessengerTest.t.sol

scripts/local/lint.sh

cam-schultz · 2023-10-19T21:48:38Z

.github/workflows/linter.yml

@@ -24,5 +24,4 @@ jobs:
            # in subdirectories. The former only checks sol files in the current directory and directories one level down.
            - name: Run Lint
              run: |
-                  cd contracts/src
-                  npx solhint '**/*.sol' --config ./.solhint.json --ignore-path ./.solhintignore --max-warnings 0
+                  ./scripts/local/lint.sh


I think we need to either cd contracts/src or modify the script to point to TELEPORTER_PATH/contracts/src/.solhint.json

yup the lint script has cd $TELEPORTER_PATH/contracts/src. Lint workflow was failing because of installing locally, and script using solhint without npx. Looking online, when installing to use for command line we should install globally. https://nodejs.org/en/blog/npm/npm-1-0-global-vs-local-installation

Ah I missed that cd. Nice find

cam-schultz

LGTM!

michaelkaplan13

👍

Matthew Lam added 6 commits October 13, 2023 21:12

revert custom errors and remove redundant teleporter sender checks

c8ec8ef

update to use error strings in require

e47a0a3

add reason string lint and lint script

f31b07f

update erc20Bridge to use require statements

5590b59

update cross chain apps to use require statements and abi bindings

6c2a6cd

cleanup error messages for require

a47b9e6

minghinmatthewlam requested review from michaelkaplan13, cam-schultz, gwen917, geoff-vball and bernard-avalabs as code owners October 16, 2023 21:37

update abi bindings

3e5d6c1

bernard-avalabs previously approved these changes Oct 17, 2023

View reviewed changes

contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol Show resolved Hide resolved

update error messages

ebc937f

minghinmatthewlam dismissed bernard-avalabs’s stale review via ebc937f October 17, 2023 22:28

minghinmatthewlam requested a review from bernard-avalabs October 17, 2023 22:28

bernard-avalabs reviewed Oct 18, 2023

View reviewed changes

contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol Outdated Show resolved Hide resolved

contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol Show resolved Hide resolved

contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol Show resolved Hide resolved

Matthew Lam added 6 commits October 18, 2023 19:25

Merge remote-tracking branch 'origin/main' into revert-custom-errors

1f1e10c

additional custom error removals

8f76809

remove additional custom errors and update bindings

1821c9e

Merge remote-tracking branch 'origin/main' into revert-custom-errors

91c7412

change to >= check

07e380e

change check to use > 0

491900d

michaelkaplan13 reviewed Oct 18, 2023

View reviewed changes

minghinmatthewlam and others added 6 commits October 18, 2023 13:55

Update contracts/src/Teleporter/TeleporterMessenger.sol

51f3da9

Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>

Update contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol

b7c17ab

Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>

Update contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol

5ba1868

Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>

Update contracts/src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol

faa2837

Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>

update lint command to match ci

e2b68a1

revert change for >= check to prevent bridging with no amount

7855a5b

Merge remote-tracking branch 'origin/main' into revert-custom-errors

75a9b0e

michaelkaplan13 previously approved these changes Oct 18, 2023

View reviewed changes

minghinmatthewlam requested a review from bernard-avalabs October 18, 2023 21:28

Merge remote-tracking branch 'origin/main' into revert-custom-errors

4104849

minghinmatthewlam dismissed michaelkaplan13’s stale review via 4104849 October 18, 2023 22:18

minghinmatthewlam requested a review from michaelkaplan13 October 18, 2023 22:18

cam-schultz reviewed Oct 19, 2023

View reviewed changes

contracts/src/Teleporter/TeleporterMessenger.sol Show resolved Hide resolved

contracts/src/Teleporter/tests/TeleporterMessengerTest.t.sol Outdated Show resolved Hide resolved

scripts/local/lint.sh Show resolved Hide resolved

rename format error message functions and use lint script for ci

c87cc97

minghinmatthewlam requested a review from cam-schultz October 19, 2023 21:46

cam-schultz reviewed Oct 19, 2023

View reviewed changes

install global instead of local because using as command line

661e408

cam-schultz approved these changes Oct 19, 2023

View reviewed changes

michaelkaplan13 approved these changes Oct 19, 2023

View reviewed changes

minghinmatthewlam merged commit 63e7132 into main Oct 19, 2023

minghinmatthewlam deleted the revert-custom-errors branch October 19, 2023 22:17

cam-schultz mentioned this pull request Oct 23, 2023

Revert back to require instead of custom errors #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert custom errors to require statements #59

Revert custom errors to require statements #59

minghinmatthewlam commented Oct 16, 2023 •

edited

Loading

bernard-avalabs left a comment

bernard-avalabs left a comment

michaelkaplan13 Oct 18, 2023

minghinmatthewlam Oct 18, 2023

michaelkaplan13 Oct 18, 2023

michaelkaplan13 left a comment

cam-schultz left a comment

cam-schultz Oct 19, 2023

minghinmatthewlam Oct 19, 2023

cam-schultz Oct 19, 2023

cam-schultz left a comment

michaelkaplan13 left a comment

Revert custom errors to require statements #59

Revert custom errors to require statements #59

Conversation

minghinmatthewlam commented Oct 16, 2023 • edited Loading

Why this should be merged

How this works

How this was tested

bernard-avalabs left a comment

Choose a reason for hiding this comment

bernard-avalabs left a comment

Choose a reason for hiding this comment

michaelkaplan13 Oct 18, 2023

Choose a reason for hiding this comment

minghinmatthewlam Oct 18, 2023

Choose a reason for hiding this comment

michaelkaplan13 Oct 18, 2023

Choose a reason for hiding this comment

michaelkaplan13 left a comment

Choose a reason for hiding this comment

cam-schultz left a comment

Choose a reason for hiding this comment

cam-schultz Oct 19, 2023

Choose a reason for hiding this comment

minghinmatthewlam Oct 19, 2023

Choose a reason for hiding this comment

cam-schultz Oct 19, 2023

Choose a reason for hiding this comment

cam-schultz left a comment

Choose a reason for hiding this comment

michaelkaplan13 left a comment

Choose a reason for hiding this comment

minghinmatthewlam commented Oct 16, 2023 •

edited

Loading