Add ERC6909 Implementation along with extensions #5394
Conversation
🦋 Changeset detectedLatest commit: 673124c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
arr00
force-pushed
the
feat/6909-impl
branch
from
a863f2b to
7d78b5a
Compare
| error ERC6909InvalidReceiver(address receiver); | ||
| error ERC6909InvalidSender(address sender); | ||
|
|
||
| mapping(uint256 id => mapping(address owner => uint256)) private _balances; |
There was a problem hiding this comment.
Why not mapping(address owner => mapping(uint256 id => uint256))? I feel is more consistent
There was a problem hiding this comment.
This is following ERC1155 impl. Given that we've strayed pretty far from storage parity at this point its probably not necessary.
| } | ||
| } | ||
| if (to != address(0)) { | ||
| _balances[id][to] += amount; |
There was a problem hiding this comment.
Just realized this can't be unchecked given it's not bound by a totalSupply as ERC20 does, which means that multiple accounts can hold more than the maximum uint256 value.
I wonder if this is an issue we should worry about. An example where it can break is for off-chain indexers who may want to track the total supply but are restricted to an uint256 (as the graph)
There was a problem hiding this comment.
ERC1155 is the same by default. Given that the ERC doesn't restrict this at all, and that it is already done in ERC1155, I think it is fine.
Fixes #????
PR Checklist
npx changeset add)