-
Notifications
You must be signed in to change notification settings - Fork 12.3k
Add Math.modExp and a Panic library
#3298
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
Changes from 49 commits
91e39eb
712a0f3
77f33ea
4683f26
7489ce4
d576641
0d78d29
5d99ed8
acb16f2
d7e81cf
85187dd
d458765
01badeb
a1c1439
d81d69d
6e8cced
fd7b8de
26a036e
4ba0a29
988c950
c08ac50
98f7994
f634aab
66a0c1f
eecd818
1583160
19ead8e
8cf355f
113e85e
4e1cf0d
6d7c154
84b285d
9e73f46
76c9afa
1ff0776
f84b1b6
fe32a38
4accc2e
cfd80e9
526d6b9
3718090
cd2f2e9
104002e
d149ea6
05aa60e
f352681
32ea4bb
2e962c8
275c959
24cd52a
50374a1
ee91836
d13e52d
969e259
e64c3f9
06220be
9137bae
81e8ba0
2b72050
2fc20d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| 'openzeppelin-solidity': minor | ||
| --- | ||
|
|
||
| `Math`: Add `modExp` function that exposes the `EIP-198` precompile. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| 'openzeppelin-solidity': minor | ||
| --- | ||
|
|
||
| `Panic`: Add a library for reverting with panic codes. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,28 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SPDX-License-Identifier: MIT | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pragma solidity ^0.8.20; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @dev Helper library for emitting standardized panic codes. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // slither-disable-next-line unused-state | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| library Panic { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant ASSERTION_ERROR = 0x1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant ARITHMETIC_UNDER_OR_OVERFLOW = 0x11; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant DIVISION_BY_ZERO = 0x12; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant ENUM_CONVERSION_OUT_OF_BOUNDS = 0x21; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant INCORRECTLY_ENCODED_STORAGE_BYTE_ARRAY = 0x22; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant POP_ON_EMPTY_ARRAY = 0x31; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant ARRAY_ACCESS_OUT_OF_BOUNDS = 0x32; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant TOO_MUCH_MEMORY_ALLOCATED = 0x41; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant ZERO_INITIALIZED_VARIABLE = 0x51; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uint256 internal constant ASSERTION_ERROR = 0x1; | |
| uint256 internal constant ARITHMETIC_UNDER_OR_OVERFLOW = 0x11; | |
| uint256 internal constant DIVISION_BY_ZERO = 0x12; | |
| uint256 internal constant ENUM_CONVERSION_OUT_OF_BOUNDS = 0x21; | |
| uint256 internal constant INCORRECTLY_ENCODED_STORAGE_BYTE_ARRAY = 0x22; | |
| uint256 internal constant POP_ON_EMPTY_ARRAY = 0x31; | |
| uint256 internal constant ARRAY_ACCESS_OUT_OF_BOUNDS = 0x32; | |
| uint256 internal constant TOO_MUCH_MEMORY_ALLOCATED = 0x41; | |
| uint256 internal constant ZERO_INITIALIZED_VARIABLE = 0x51; | |
| /// @dev Called `assert` with an argument that evaluates to `false`. | |
| uint256 internal constant ASSERT = 0x1; | |
| /// @dev An arithmetic operation resulted in underflow or overflow. | |
| uint256 internal constant UNDER_OVERFLOW = 0x11; | |
| /// @dev Division or modulo operation by 0 (i.e. `x / 0` or `x % 0`). | |
| uint256 internal constant DIVISION_BY_ZERO = 0x12; | |
| /// @dev A number greater than 255 or negative was wrapped in an `enum`. | |
| uint256 internal constant ENUM_CONVERSION_ERROR = 0x21; | |
| /// ??? | |
| uint256 internal constant STORAGE_ENCODING_ERROR = 0x22; | |
| /// @dev Attempted to `pop` an element out of an empty array. | |
| uint256 internal constant EMPTY_ARRAY_POP = 0x31; | |
| /// @dev Attempted to access an out-of-bound index at an empty array | |
| /// (i.e. `x[i]` where `i >= x.length` or `i < 0`). | |
| uint256 internal constant ARRAY_OUT_OF_BOUNDS = 0x32; | |
| /// @dev If you allocate too much memory or create an array that is too large. | |
| uint256 internal constant RESOURCE_ERROR = 0x41; | |
| /// @dev Called an uninitialized internal function pointer. | |
| uint256 internal constant INVALID_INTERNAL_FUNCTION = 0x51; |
Note I don't know how to trigger the 0x22 code. Also, I couldn't find what "too large" means in code 0x41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the name for hardhat helpers, which is handy for testing. I'm open to using Solidity's names, but then testing will be a bit more confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now anyone can call Panic.code(0xff) when 0xff is not in the "official" list (I'm not sure what "official" even means in this context).
I'm not sure how frequently Solidity adds (or changes) the panic codes, but imagine the following situation:
- We release this
Paniclibrary - Some protocol start using
Panic.code(0x420) - Solidity releases a new panic code in 0.9.0 using the same code (
0x420)
Wouldn't it be bad that the same revert code has different meanings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we can generate this library procedurally wth the codes, such that users can do:
Panic.ASSERTION_ERROR();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too worried about users emitting random panic code ... but I'm not fundamentally opposed to making panic(uint256) private and having functions like
function TooMuchMemoryAllocated() internal pure { _panic(0x41); }
We would have to make them CamelCase though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing, if new panic code are ever added, the current version would allow users to use them without us having to do a release.
I'm also thinking, what if some L2 whant to use specific panic codes for some crosschain stuff. We won't want to add them to the library, but users will want to emit them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would have to make them CamelCase though.
Agree with this syntax.
The thing, if new panic code are ever added, the current version would allow users to use them without us having to do a release.
Right, good point. If the goal is to provide a workaround then I would be fine if we release the panic(uint256) function, although I'd suggest naming it _panic(uint256) and making it internal instead.
I know this is kind of the same thing but I feel the design of the library API is more friendly if we keep each panic code function and also the custom _panic for the following reasons:
- The developers will have defined functions out of the box without the need to check the internal constants.
- When using functions in editors like VSCode, the NatSpec shows up. Developers will have each panic code explained in their IDEs (I think this doesn't happen with the constants)
- I realized it doesn't make sense for a protocol to rely on external contracts returning a panic code, it's easily exploitable and not 100% reliable anyway. It's worth keeping it but underscoring
_panichelps signaling that the function is not intended for direct access but as a fallback for custom use cases.
Would you agree with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter rules are not ok with that, and I think we should follow them:
- Constant must be SNAKE_CASE
- Library internal functions should not have leading underscore
The developers will have defined functions out of the box without the need to check the internal constants.
I'm not sure that is a good point. When testing panic code with hardhat you do
expect(...).to.be.revertedWithPanic(PANIC_CODE.ARITHMETIC_UNDER_OR_OVERFLOW)
you don't to
expect(...).to.be.revertedWithPanicArithmeticUnderOrOverflow();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current version is simple and effective. It's targetting advanced users anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that is a good point. When testing panic code with hardhat you do
expect(...).to.be.revertedWithPanic(PANIC_CODE.ARITHMETIC_UNDER_OR_OVERFLOW)
Fair
I think the cureent version is simple and effective. It's targetting advanced users anyway.
We should document the function then. Currently it allows panicking with anything it I think it needs some usage instructions in both the contracts NatSpec and the panic funciton itself
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| const { ethers } = require('hardhat'); | ||
| const { expect } = require('chai'); | ||
| const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); | ||
| const { PANIC_CODES } = require('@nomicfoundation/hardhat-chai-matchers/panic'); | ||
|
|
||
| async function fixture() { | ||
| return { mock: await ethers.deployContract('$Panic') }; | ||
| } | ||
|
|
||
| describe('Panic', function () { | ||
| beforeEach(async function () { | ||
| Object.assign(this, await loadFixture(fixture)); | ||
| }); | ||
|
|
||
| for (const [name, code] of Object.entries(PANIC_CODES)) { | ||
| describe(name, function () { | ||
| it('exposes panic code as constant', async function () { | ||
| expect(await this.mock.getFunction(`$${name}`)()).to.equal(code); | ||
| }); | ||
|
|
||
| it(`throw panic code ${ethers.toBeHex(code)}`, async function () { | ||
| await expect(this.mock.$panic(code)).to.be.revertedWithPanic(code); | ||
| }); | ||
| }); | ||
| } | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.