feat: nft bridge betwen l1 and optimism#2577
feat: nft bridge betwen l1 and optimism#2577sam-goldman-zz wants to merge 1 commit intoethereum-optimism:developfrom
Conversation
🦋 Changeset detectedLatest commit: e307559 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
30d62d9 to
e307559
Compare
|
@tynes It's all ready for review! |
| event StandardL2ERC721Created(address indexed _l1Token, address indexed _l2Token); | ||
|
|
||
| // Address of the L2 ERC721 Bridge. It currently exists only on Optimistic Kovan. | ||
| address public constant L2_ERC721_BRIDGE = 0xA779A0cA89556A9dffD47527F0aad1c2e0d66e46; |
There was a problem hiding this comment.
This should be turned into a constructor argument
|
|
||
| // Maps an L2 ERC721 token address to a boolean that returns true if the token was created | ||
| // with the L2StandardERC721Factory. | ||
| mapping(address => bool) public isStandardERC721; |
There was a problem hiding this comment.
What is this mapping for exactly? Because developers can deploy custom L2 ERC721s that implement the correct interface and not be included in this mapping. Is is supposed to signify a "token list"?
There was a problem hiding this comment.
The purpose is just to give frontends an easy way to determine if an L2 ERC721 is a Standard ERC721. In other words, it gives frontends an easy way to tell users, "this L2 ERC721 contract doesn't implement any custom (i.e. potentially dangerous) functionality". The mapping isn't supposed to signify a token list, since that can include custom ERC721s too.
|
|
||
| // Maps an L1 ERC721 token address to an L2 Standard ERC721 token address. This mapping can | ||
| // only be updated once per L1 ERC721 token. | ||
| mapping(address => address) public standardERC721Mapping; |
There was a problem hiding this comment.
Same comment as above - I feel like an offchain tokenlist is better for this because using this factory is permissionless so its not the best indicator of legitimacy
There was a problem hiding this comment.
The purpose of this mapping isn't actually to maintain an on-chain token list. The mapping is there to ensure that there's one, and only one, L2 Standard ERC721 contract for each L1 ERC721 contract (which is enforced here). Since these standard contracts implement the exact same functionality, there's no need to have more than one for each L1 contract. Also, this mapping makes it easy for frontends to find out which L2 Standard ERC721 corresponds to an L1 ERC721, which makes it easy to know which L2 contract to bridge to.
|
Replaced by #2662 |
guoshijiang
left a comment
There was a problem hiding this comment.
sdk test erc721 bridge code also need write
Description
An ERC721 bridge between Mainnet and Optimism. The issue describing the bridge can be found here.
After the PR has been reviewed, the address of the L2ERC721Bridge needs to be updated from its placeholder address (0xA77...) here and here. Once this is done, the PR will be ready to merge.