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

style: use custom errors instead #56

Merged
merged 4 commits into from
Dec 12, 2022
Merged

Conversation

edison0xyz
Copy link
Contributor

No description provided.

@edison0xyz edison0xyz requested a review from zlace0x December 10, 2022 15:54
src/Funnel.sol Outdated Show resolved Hide resolved
src/Funnel.sol Outdated Show resolved Hide resolved
src/Funnel.sol Outdated Show resolved Hide resolved
@edison0xyz
Copy link
Contributor Author

Pushed another commit that addresses most of the comments. And also compared the gas usage.

Before PR:

✹ ✚
[⠆] Compiling...
No files changed, compilation skipped

Running 2 tests for test/MockSpenderReceiver.t.sol:MockSpenderReceiverTest
[PASS] testSupportInterfaceReceiver() (gas: 5598)
[PASS] testSupportInterfaceSpender() (gas: 6567)
Test result: ok. 2 passed; 0 failed; finished in 3.09ms

Running 11 tests for test/FunnelFactory.t.sol:FunnelFactoryTest
[PASS] testDeployFunnelForDifferentTokens() (gas: 298301)
[PASS] testDeployFunnelForToken() (gas: 156993)
[PASS] testDeployFunnelForTokenRevertsIfAlreadyDeployed() (gas: 156707)
[PASS] testDeployFunnelFromDifferentFactory() (gas: 558198)
[PASS] testGetFunnelForTokenRevertsIfNotDeployed() (gas: 12793)
[PASS] testIsFunnelFalseForDeployedFunnelFromDifferentFactory() (gas: 563350)
[PASS] testIsFunnelFalseForNonFunnel() (gas: 10875)
[PASS] testIsFunnelFalseForUndeployedFunnel() (gas: 8329)
[PASS] testIsFunnelTrueForDeployedFunnel() (gas: 155345)
[PASS] testNoCodeTokenReverts() (gas: 15399)
[PASS] testTransferFromFunnel() (gas: 294377)
Test result: ok. 11 passed; 0 failed; finished in 41.16ms

Running 18 tests for test/FunnelERC20.t.sol:FunnelERC20Test
[PASS] testApproveFuzzing(uint256) (runs: 256, μ: 118180, ~: 119891)
[PASS] testApproveMaxWithTransfer() (gas: 138686)
[PASS] testApproveWithTransferFuzzing(uint256,uint256) (runs: 256, μ: 148771, ~: 150398)
[PASS] testBalanceOfOnMint(uint256) (runs: 256, μ: 66292, ~: 67015)
[PASS] testBalanceOfOnMintLargeAmount() (gas: 66981)
[PASS] testBalanceOfOnTransfer(uint256,uint256) (runs: 256, μ: 104454, ~: 107372)
[PASS] testBalanceOfOnTransferLargeAmount() (gas: 87476)
[PASS] testBalanceOfReflectsSlot(uint256) (runs: 256, μ: 322, ~: 322)
[PASS] testFailApproveWithTransferInsufficientApproval() (gas: 100475)
[PASS] testFailApprovewithTransferInsufficientBalance() (gas: 108378)
[PASS] testFailTransferExceedsBalance(uint256) (runs: 256, μ: 40582, ~: 40682)
[PASS] testInitialAllowance() (gas: 25026)
[PASS] testTotalSupply(uint256) (runs: 256, μ: 42945, ~: 42933)
[PASS] testTransferFullBalance() (gas: 61369)
[PASS] testTransferFuzzing(uint256) (runs: 256, μ: 69128, ~: 69678)
[PASS] testTransferHalfBalance() (gas: 66255)
[PASS] testTransferOneToken() (gas: 64089)
[PASS] testTransferZeroTokens() (gas: 41433)
Test result: ok. 18 passed; 0 failed; finished in 44.70ms

Running 41 tests for test/Funnel.t.sol:FunnelTest
[PASS] testApprove() (gas: 144722)
[PASS] testApproveRenewable() (gas: 96800)
[PASS] testApproveRenewableAndCall() (gas: 92507)
[PASS] testApproveRenewableAndCallRevertNonContract() (gas: 84837)
[PASS] testApproveRenewableAndCallRevertNonReceiver() (gas: 85743)
[PASS] testBaseToken() (gas: 9949)
[PASS] testBaseTokenAllowance() (gas: 103891)
[PASS] testClearRenewableAllowanceOnNormalApprove() (gas: 145917)
[PASS] testExecuteMetaTransactionApproveRenewable() (gas: 136224)
[PASS] testExecuteMetaTransactionTransfer() (gas: 97013)
[PASS] testFailExecuteMetaTransactionBadNonce() (gas: 47605)
[PASS] testFailExecuteMetaTransactionReplayProtection() (gas: 143215)
[PASS] testFailPermitBadDeadline() (gas: 46660)
[PASS] testFailPermitBadNonce() (gas: 46695)
[PASS] testFailPermitPastDeadline() (gas: 15810)
[PASS] testFailPermitRenewableBadDeadline() (gas: 48805)
[PASS] testFailPermitRenewableBadNonce() (gas: 48754)
[PASS] testFailPermitRenewablePastDeadline() (gas: 20145)
[PASS] testFailPermitRenewableReplay() (gas: 125640)
[PASS] testFailPermitReplay() (gas: 123582)
[PASS] testFallbackToBaseToken() (gas: 25657)
[PASS] testInfiniteApproveTransferFrom() (gas: 134068)
[PASS] testInsufficientAllowance() (gas: 29392)
[PASS] testOverflow() (gas: 116539)
[PASS] testOverflow2() (gas: 116545)
[PASS] testOverflow3() (gas: 129944)
[PASS] testOverriddenName() (gas: 20769)
[PASS] testPermit() (gas: 128775)
[PASS] testPermitRenewable() (gas: 130791)
[PASS] testRecoveryRateCasting() (gas: 85939)
[PASS] testRecoveryRateExceeded() (gas: 13474)
[PASS] testRecoveryRateExceeded2() (gas: 13428)
[PASS] testRenewableAllowanceTransferFrom() (gas: 155321)
[PASS] testRenewableMaxAllowance() (gas: 98292)
[PASS] testSupportInterface() (gas: 5766)
[PASS] testSupportsInterfacePayable() (gas: 5836)
[PASS] testSupportsInterfaceProxy() (gas: 5763)
[PASS] testTransfer() (gas: 61980)
[PASS] testTransferFromAndCall() (gas: 138831)
[PASS] testTransferFromAndCallRevertNonContract() (gas: 113532)
[PASS] testTransferFromAndCallRevertNonReceiver() (gas: 111933)
Test result: ok. 41 passed; 0 failed; finished in 44.75ms

After custom errors:

[⠆] Compiling...
[⠑] Compiling 7 files with 0.8.17
[⠃] Solc 0.8.17 finished in 2.36s
Compiler run successful

Running 2 tests for test/MockSpenderReceiver.t.sol:MockSpenderReceiverTest
[PASS] testSupportInterfaceReceiver() (gas: 5598)
[PASS] testSupportInterfaceSpender() (gas: 6567)
Test result: ok. 2 passed; 0 failed; finished in 2.63ms

Running 11 tests for test/FunnelFactory.t.sol:FunnelFactoryTest
[PASS] testDeployFunnelForDifferentTokens() (gas: 298301)
[PASS] testDeployFunnelForToken() (gas: 156993)
[PASS] testDeployFunnelForTokenRevertsIfAlreadyDeployed() (gas: 156707)
[PASS] testDeployFunnelFromDifferentFactory() (gas: 558192)
[PASS] testGetFunnelForTokenRevertsIfNotDeployed() (gas: 12793)
[PASS] testIsFunnelFalseForDeployedFunnelFromDifferentFactory() (gas: 563344)
[PASS] testIsFunnelFalseForNonFunnel() (gas: 10875)
[PASS] testIsFunnelFalseForUndeployedFunnel() (gas: 8329)
[PASS] testIsFunnelTrueForDeployedFunnel() (gas: 155345)
[PASS] testNoCodeTokenReverts() (gas: 15399)
[PASS] testTransferFromFunnel() (gas: 294377)
Test result: ok. 11 passed; 0 failed; finished in 4.31ms

Running 41 tests for test/Funnel.t.sol:FunnelTest
[PASS] testApprove() (gas: 144722)
[PASS] testApproveRenewable() (gas: 96800)
[PASS] testApproveRenewableAndCall() (gas: 92507)
[PASS] testApproveRenewableAndCallRevertNonContract() (gas: 84765)
[PASS] testApproveRenewableAndCallRevertNonReceiver() (gas: 85695)
[PASS] testBaseToken() (gas: 9949)
[PASS] testBaseTokenAllowance() (gas: 103891)
[PASS] testClearRenewableAllowanceOnNormalApprove() (gas: 145917)
[PASS] testExecuteMetaTransactionApproveRenewable() (gas: 136224)
[PASS] testExecuteMetaTransactionTransfer() (gas: 97013)
[PASS] testFailExecuteMetaTransactionBadNonce() (gas: 47605)
[PASS] testFailExecuteMetaTransactionReplayProtection() (gas: 143215)
[PASS] testFailPermitBadDeadline() (gas: 46660)
[PASS] testFailPermitBadNonce() (gas: 46695)
[PASS] testFailPermitPastDeadline() (gas: 15729)
[PASS] testFailPermitRenewableBadDeadline() (gas: 48805)
[PASS] testFailPermitRenewableBadNonce() (gas: 48754)
[PASS] testFailPermitRenewablePastDeadline() (gas: 20064)
[PASS] testFailPermitRenewableReplay() (gas: 125640)
[PASS] testFailPermitReplay() (gas: 123582)
[PASS] testFallbackToBaseToken() (gas: 25657)
[PASS] testInfiniteApproveTransferFrom() (gas: 134068)
[PASS] testInsufficientAllowance() (gas: 29392)
[PASS] testOverflow() (gas: 116539)
[PASS] testOverflow2() (gas: 116545)
[PASS] testOverflow3() (gas: 129944)
[PASS] testOverriddenName() (gas: 20769)
[PASS] testPermit() (gas: 128775)
[PASS] testPermitRenewable() (gas: 130791)
[PASS] testRecoveryRateCasting() (gas: 85939)
[PASS] testRecoveryRateExceeded() (gas: 13474)
[PASS] testRecoveryRateExceeded2() (gas: 13491)
[PASS] testRenewableAllowanceTransferFrom() (gas: 155321)
[PASS] testRenewableMaxAllowance() (gas: 98292)
[PASS] testSupportInterface() (gas: 5766)
[PASS] testSupportsInterfacePayable() (gas: 5836)
[PASS] testSupportsInterfaceProxy() (gas: 5763)
[PASS] testTransfer() (gas: 61980)
[PASS] testTransferFromAndCall() (gas: 138831)
[PASS] testTransferFromAndCallRevertNonContract() (gas: 113465)
[PASS] testTransferFromAndCallRevertNonReceiver() (gas: 111885)
Test result: ok. 41 passed; 0 failed; finished in 9.57ms

Running 18 tests for test/FunnelERC20.t.sol:FunnelERC20Test
[PASS] testApproveFuzzing(uint256) (runs: 256, μ: 117869, ~: 119891)
[PASS] testApproveMaxWithTransfer() (gas: 138686)
[PASS] testApproveWithTransferFuzzing(uint256,uint256) (runs: 256, μ: 147948, ~: 150398)
[PASS] testBalanceOfOnMint(uint256) (runs: 256, μ: 65772, ~: 67015)
[PASS] testBalanceOfOnMintLargeAmount() (gas: 66981)
[PASS] testBalanceOfOnTransfer(uint256,uint256) (runs: 256, μ: 104370, ~: 107372)
[PASS] testBalanceOfOnTransferLargeAmount() (gas: 87476)
[PASS] testBalanceOfReflectsSlot(uint256) (runs: 256, μ: 322, ~: 322)
[PASS] testFailApproveWithTransferInsufficientApproval() (gas: 100475)
[PASS] testFailApprovewithTransferInsufficientBalance() (gas: 108378)
[PASS] testFailTransferExceedsBalance(uint256) (runs: 256, μ: 40575, ~: 40682)
[PASS] testInitialAllowance() (gas: 25026)
[PASS] testTotalSupply(uint256) (runs: 256, μ: 42753, ~: 42933)
[PASS] testTransferFullBalance() (gas: 61369)
[PASS] testTransferFuzzing(uint256) (runs: 256, μ: 68547, ~: 69678)
[PASS] testTransferHalfBalance() (gas: 66255)
[PASS] testTransferOneToken() (gas: 64089)
[PASS] testTransferZeroTokens() (gas: 41433)
Test result: ok. 18 passed; 0 failed; finished in 46.14ms

Generally we saved 20-80 gas for functions where there is an error thrown.

src/Funnel.sol Outdated Show resolved Hide resolved
Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>
@edison0xyz edison0xyz merged commit 0c44644 into chore/natspec Dec 12, 2022
@edison0xyz edison0xyz deleted the fix/audit-error branch December 12, 2022 08:54
zlace0x added a commit that referenced this pull request Dec 12, 2022
* style: grouped functions

* forge install: openzeppelin-contracts

* chore: updates to lib

* chore: natspec comments for contracts

* chore: closes #39 and #40

* chore: removed unused solmate

* refactor: moved saturatingAdd to mathUtil

* chore: cleanup

* Apply suggestions from code review

Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

* fix: resolve some comments

* doc: generated docs under doc/

* chore: cleanups on comments

* style: use custom errors instead (#56)

* style: use custom errors instead

* fix: structured files for custom errors

* style: change INITIAL values to upper-case

* Update src/Funnel.sol

Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

* chore: fix comments slashes

* fix: apply linter version changes

Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
Co-authored-by: zlace0x <zlace0x@gmail.com>
Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>
zlace0x added a commit that referenced this pull request Dec 12, 2022
* fix: addresses #32, allowance() amount should reflect transferable amounts for transferFrom() and transfer()

* fix: comment contradition #43

* wip: added overflow test cases according to #30, identified area of overflow

* fix: potential fix for #30

* refactor: adhere to checks-effects-interaction pattern #35

* fix: make allowance() external #45

* fix: dropped virtual from permit, permitRenewable #44

* refactor: added require reason

* chore: added gas reports

* fix: solidity compiler for deploy script

* Chore/natspec (#53)

* style: grouped functions

* forge install: openzeppelin-contracts

* chore: updates to lib

* chore: natspec comments for contracts

* chore: closes #39 and #40

* chore: removed unused solmate

* refactor: moved saturatingAdd to mathUtil

* chore: cleanup

* Apply suggestions from code review

Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

* fix: resolve some comments

* doc: generated docs under doc/

* chore: cleanups on comments

* style: use custom errors instead (#56)

* style: use custom errors instead

* fix: structured files for custom errors

* style: change INITIAL values to upper-case

* Update src/Funnel.sol

Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

* chore: fix comments slashes

* fix: apply linter version changes

Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
Co-authored-by: zlace0x <zlace0x@gmail.com>
Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

Co-authored-by: Edison <6057323+edison0xyz@users.noreply.github.com>
Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
zlace0x added a commit that referenced this pull request Dec 12, 2022
* fix: resolves #48 resolves 45 resolves #44 resolves #41 resolves #38

* fix: closes #34

* fix: zero address validation and make functions external. Closes #42 closes #45

* fixed pragma. closes #36

* fix as 0.8.17

* fix: remove solmate

* refactor: PR comments
- reorder imports, fix qoutes
- removed virtual for _checkOn* functions
- added virtual to supportsInterface
- fix compiler version to 0.8.17

* fix: addresses #32, allowance() amount should reflect transferable amounts for transferFrom() and transfer()

* fix: comment contradition #43

* wip: added overflow test cases according to #30, identified area of overflow

* fix: potential fix for #30

* refactor: adhere to checks-effects-interaction pattern #35

* fix: make allowance() external #45

* fix: dropped virtual from permit, permitRenewable #44

* refactor: added require reason

* chore: added gas reports

* fix: solidity compiler for deploy script

* Chore/natspec (#53)

* style: grouped functions

* forge install: openzeppelin-contracts

* chore: updates to lib

* chore: natspec comments for contracts

* chore: closes #39 and #40

* chore: removed unused solmate

* refactor: moved saturatingAdd to mathUtil

* chore: cleanup

* Apply suggestions from code review

Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

* fix: resolve some comments

* doc: generated docs under doc/

* chore: cleanups on comments

* style: use custom errors instead (#56)

* style: use custom errors instead

* fix: structured files for custom errors

* style: change INITIAL values to upper-case

* Update src/Funnel.sol

Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

* chore: fix comments slashes

* fix: apply linter version changes

Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
Co-authored-by: zlace0x <zlace0x@gmail.com>
Co-authored-by: zlace0x <81418809+zlace0x@users.noreply.github.com>

Co-authored-by: Edison <6057323+edison0x@users.noreply.github.com>
Co-authored-by: Edison <6057323+edison0xyz@users.noreply.github.com>
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.

2 participants