Skip to content
Merged
Changes from 1 commit
Commits
Show all changes
122 commits
Select commit Hold shift + click to select a range
4307e5b
Replace error strings with custom errors
ernestognw May 18, 2023
ddf387c
Lint
ernestognw May 18, 2023
0800e32
Finish original list of errors
ernestognw May 19, 2023
199093b
Finish missing require statements
ernestognw May 19, 2023
d3703bd
Self review
ernestognw May 19, 2023
11931ca
Finish revert statements
ernestognw May 19, 2023
bb3a12f
Applied spreadsheet suggestion
ernestognw May 19, 2023
9f58996
Refactor Address.sol
ernestognw May 19, 2023
a76e4b6
Finish custom errors replacement
ernestognw May 19, 2023
c65b76b
Fix SignatureChecker
ernestognw May 20, 2023
f58c06b
Add account to `GovernorAlreadyCastVote` error
ernestognw May 21, 2023
810468d
Finish access, finance, and governance testing
ernestognw May 22, 2023
adbe841
Fix proxy tests
ernestognw May 22, 2023
c29e041
First round of reviews
ernestognw May 22, 2023
bc00094
Fix tests for ERC20 and ERC1155 tokens
ernestognw May 23, 2023
faf33a4
Lint
ernestognw May 23, 2023
cdbea1e
Bump Pragma to 0.8.19
ernestognw May 23, 2023
77eee99
Finish token tests
ernestognw May 23, 2023
ba42df6
Fix ERC20Capped
ernestognw May 23, 2023
c31e10d
Fix Address tests
ernestognw May 23, 2023
b3b7e08
Advancements on utils
ernestognw May 24, 2023
960066b
Fix utils test
ernestognw May 24, 2023
b815a4f
Remove unnecessary test file
ernestognw May 24, 2023
99347e4
Fix conflicts with master
ernestognw May 24, 2023
d7d3f90
Lint
ernestognw May 24, 2023
5bc0812
Add changeset
ernestognw May 24, 2023
3276d35
Fix generation script
ernestognw May 24, 2023
23a7be2
Fix flaky Create2 test
ernestognw May 24, 2023
3f4f84a
Update comment in Ownable
ernestognw May 24, 2023
3dc21e2
Add `Unset` state to TimelockController
ernestognw May 24, 2023
1c119f7
Replace `GovernorMissingETA` with `GovernorProposalNotQueued`
ernestognw May 24, 2023
6541481
Revert interface changes to MinimalForwarder
ernestognw May 24, 2023
9c71d9c
Remove ERC-6093 from token interfaces
ernestognw May 24, 2023
5f8417b
Replace `msg.sender` with `_msgSender()` in ERC721Wrapper
ernestognw May 24, 2023
55f0eef
Make `onRevert` view visilibity
ernestognw May 24, 2023
5fe9511
Change Ownable2Step's error for invalid ownership acceptance
ernestognw May 24, 2023
c927bc3
Replace SafeERC20's domain
ernestognw May 25, 2023
0a5bb3f
Merge branch 'next-v5.0' into lib-839-custom-errors
ernestognw May 25, 2023
eac424e
Lint fix
ernestognw May 25, 2023
e11f4fa
Merge branch 'next-v5.0' into lib-839-custom-errors
ernestognw May 29, 2023
5dc5ada
Fix checkpoint tests
ernestognw May 29, 2023
44535a2
Enabled all tests
ernestognw May 29, 2023
0d1d6c0
Remove GovernorDuplicatedProposal error
ernestognw May 29, 2023
07d4fc9
Merge branch 'next-v5.0' into lib-839-custom-errors
ernestognw May 29, 2023
058b672
Lint
ernestognw May 29, 2023
1d63212
Update contracts/governance/IGovernor.sol
ernestognw May 30, 2023
276331c
Update contracts/utils/Address.sol
ernestognw May 30, 2023
28e2e16
Round of review
ernestognw May 30, 2023
2b0ce34
Merge branch 'next-v5.0' into lib-839-custom-errors
ernestognw May 30, 2023
4946dd9
Rename AccessContol renounce error and parameter
ernestognw May 31, 2023
3173d53
Rename `Unsuccessful` -> `Failed`
ernestognw May 31, 2023
6c8d0e1
Merge Address failed call errors
ernestognw May 31, 2023
cb79ab3
Rename *FailedLowLevelCall to *FailedCall
ernestognw May 31, 2023
be68001
Round of review
ernestognw Jun 1, 2023
f02af6e
Change token paused errors
ernestognw Jun 1, 2023
2d735e6
Fix tests
ernestognw Jun 1, 2023
2ecfd14
Round of review
ernestognw Jun 1, 2023
b3af988
Round of review
ernestognw Jun 1, 2023
c862ca5
More review
ernestognw Jun 1, 2023
5069bdf
Fixed example in StorageSlot.sol
ernestognw Jun 1, 2023
48fe940
Changed Checkpoint error
ernestognw Jun 1, 2023
c15fbf8
Added value to StringsInsufficientHexLength
ernestognw Jun 1, 2023
a10ee8b
Added `useCheckedNonce` for Nonces
ernestognw Jun 1, 2023
8d62e91
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 1, 2023
861d341
Replace ERC20DecreasedAllowance for SafeERC20
ernestognw Jun 1, 2023
61a6026
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 2, 2023
c796d15
Improved MinimalForwarder errors
ernestognw Jun 2, 2023
6a9c40f
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 2, 2023
a4063c9
Review
ernestognw Jun 2, 2023
5876d1c
Remove mutable variables in ERC1155 tests
ernestognw Jun 2, 2023
6d8c498
Remove unnecessary error
ernestognw Jun 2, 2023
219923b
Rename `Inexistent` to `Nonexistent`
ernestognw Jun 2, 2023
1fa5acc
Rename AccessControlDefaultAdminRules errors
ernestognw Jun 2, 2023
602df5a
Overflown -> Overflowed
ernestognw Jun 2, 2023
4a14bec
Simplified SafeCast errors
ernestognw Jun 2, 2023
ff12505
Review round
ernestognw Jun 2, 2023
65ad2e5
Lint
ernestognw Jun 2, 2023
91bdb7c
Mooar review
ernestognw Jun 3, 2023
a33a4c8
Revert ERC721 changes
ernestognw Jun 3, 2023
2253c40
Rename Paused errors by using a library
ernestognw Jun 3, 2023
892dbcc
Lint
ernestognw Jun 3, 2023
028f383
Fix tests
ernestognw Jun 5, 2023
9c70af8
Review suggestions
ernestognw Jun 5, 2023
7647774
Move Pausable errors to its own file
ernestognw Jun 6, 2023
45ce67f
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 6, 2023
11092da
Moar suggestions
ernestognw Jun 6, 2023
6009844
Moar suggestions
ernestognw Jun 6, 2023
55433e7
Improved custom error matcher
ernestognw Jun 7, 2023
4dac3c7
Lint
ernestognw Jun 7, 2023
aa44323
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 7, 2023
cb4594f
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 7, 2023
6df52b0
Simplify custom error matcher regex
ernestognw Jun 7, 2023
ce83461
Attempt to remove the optimizations branch check for custom errors
ernestognw Jun 7, 2023
e5475a2
Simplify custom error matcher
ernestognw Jun 8, 2023
03f1152
Revert ERC1155Supply _update order
ernestognw Jun 8, 2023
7e320a7
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 8, 2023
0e158b5
Update GUIDELINES.md
ernestognw Jun 8, 2023
f1717a6
Use verifyCallResult in Timelock
ernestognw Jun 9, 2023
db4bcf9
Fix TimelockController tests
ernestognw Jun 9, 2023
e0949a3
Add domain to DoubleEndedQueue errors
ernestognw Jun 9, 2023
46904d7
Suggestions
ernestognw Jun 9, 2023
3a74c47
Change Pausable error names
ernestognw Jun 9, 2023
7bf1afc
Remove address context from UUPSUnauthorizedCallContext
ernestognw Jun 9, 2023
13dd54a
More suggestions
ernestognw Jun 9, 2023
c97d95c
Remove errorPrefix in AccessControl tests
Amxx Jun 9, 2023
fc1e4b0
Remove errorPrefix in ERC20 tests
Amxx Jun 9, 2023
27b15e3
move ERC20Permit.test.js out of draft
Amxx Jun 9, 2023
2ad6e65
Remove errorPrefix in ERC721 tests
Amxx Jun 9, 2023
9304b9b
Fix test
ernestognw Jun 9, 2023
d4f7071
Remove Pausable.errors.sol library
ernestognw Jun 9, 2023
88cf256
Applied suggestions
ernestognw Jun 9, 2023
4432ee6
More suggestions
ernestognw Jun 9, 2023
b2aaf9e
Remove _custom revert functions
ernestognw Jun 9, 2023
d5815a8
Fix tests
ernestognw Jun 9, 2023
d4e27a0
increase gas for test
frangio Jun 9, 2023
73ac6dd
document unspecified revert, and add custom error check
Amxx Jun 12, 2023
8994817
document bubbling of panic code
Amxx Jun 12, 2023
c1c6a75
Update contracts/utils/Address.sol
ernestognw Jun 12, 2023
3da76b0
Update test/helpers/customError.js
ernestognw Jun 12, 2023
4aa35c8
Remove `bitsize` from SafeCastOverflowed cast errors
ernestognw Jun 12, 2023
007d6b0
Lint
ernestognw Jun 12, 2023
1cd28fe
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove unnecessary error
  • Loading branch information
ernestognw committed Jun 2, 2023
commit 6d8c49813e38a82be7e266150ee4f97e766eac07
7 changes: 1 addition & 6 deletions contracts/token/ERC1155/extensions/ERC1155Supply.sol
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,8 @@ abstract contract ERC1155Supply is ERC1155 {
for (uint256 i = 0; i < ids.length; ++i) {
uint256 id = ids[i];
uint256 amount = amounts[i];
uint256 supply = _totalSupply[id];
if (supply < amount) {
revert ERC1155InsufficientBalance(from, supply, amount, id);
}
_totalSupply[id] -= amount;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that ok ?

If the amount is greater then the totalSupply, this will revert without a message, and we won't get any feedback.
Since this is done BEFORE the super call, we won't have the "insufficient balance" error thrown.

Maybe we want this operation to be unchecked, knowing that any overflow here would necessarily be the result of an amount that is above the balance, and thus rely on the super call to trigger the error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would necessarily be the result of an amount that is above the balance

It's not necessarily, we had this require statement because we are concerned that if the Supply module is added in an upgrade it will not be in sync with balances.

Since this is done BEFORE the super call, we won't have the "insufficient balance" error thrown.

This is true though, shouldn't we call _update first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion and I agreed with @frangio here on doing it this way.

Still, I think the take of using the InsufficientBalance error is an excellent reason to keep it. I'm leaving this open but my standing is that both approaches have advantages and disadvantages. A summary would be:

Keeping ERC1155InsufficientBalance: Better error information, more overhead.
Relying on Solidity checked arithmetic: Less information, less overhead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping here my suggestion was that we should be caling _update before we update the supply. This means that the error due to insufficient balance will be emitted by _update, and the checked arithmetic is used for unexpected errors due to out of sync balances and supply.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bumping, I think it makes sense. Just changed the order as suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just changed the order as suggested.

Just to clarify, you didn't do this because it turns out that it introduced reentrancy risk right? We should leave this as is and do a follow up PR I think.

Copy link
Collaborator

@Amxx Amxx Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what do we do here ? I'd personally go the ERC1155InsufficientBalance way for now, and possibly change that if we refactor the _update function in a further PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal was to leave it as it is now, with checked arithmetic, and later do the update refactor.

If you guys prefer to have the custom error in the meantime, that's fine if we remember to change it.

unchecked {
// Overflow not possible: amounts[i] <= totalSupply(i)
_totalSupply[id] = supply - amount;
// Overflow not possible: sum(amounts[i]) <= sum(totalSupply(i)) <= totalSupplyAll
totalBurnAmount += amount;
}
Expand Down