Skip to content
This repository has been archived by the owner on Apr 12, 2021. It is now read-only.

Proof-of-concept ERC721 Bridge Implementation #325

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Proof-of-concept ERC721 Bridge Implementation #325

wants to merge 8 commits into from

Conversation

azf20
Copy link

@azf20 azf20 commented Mar 14, 2021

Description

  • This introduces an example ERC721 bridging implementation
  • The base ERC721 contracts are slightly amended from OpenZeppelin's latest implementation of the standard
  • There is an assumption in the existing ERC20 implementation that the source contract will always be on L1, with tokens depositing to L2. The naming convention in this ERC721 implementation doesn't make an assumption which side (Gateway / Deposited) is on L1 and which is on L2
  • This implementation makes the assumption that the source ERC721 contract has implemented the optional Metadata standard and so implements function tokenURI(uint256 _tokenId) external view returns (string); (this is implemented at the Abstract contract level)

Additionally this adds init() tests to the DepositedERC20 contract as they were quite transferable.

Questions

  • I tried to follow the patterns implemented for the ERC20 bridge as closely as possible
  • The OpenZeppelin ERC721 implementation is quite a lot heavier than the UniswapV2 standard used for ERC20 in this repo. Open to amending or slimming this down if there are other preferred reference implementations.
  • I had an issue setting bytes on smoddit, it seems like that is currently unsupported?

Metadata

Contributing Agreement

azf20 added 2 commits March 14, 2021 02:56
@azf20
contracts compiling
a354c16 
- opinionated tokenURI implementation
@azf20
Testing ERC721 gateway
22493a3 
- Add test ERC721 helper
- Tests for Gateway & Deposited ERC721
- Add same initialisation test to L2DepositedERC20
platocrat
platocrat reviewed Mar 14, 2021
View reviewed changes
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_DepositedERC721.sol
{}

/**
* @dev Initialize this contract with the token gateway address on the otehr side.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: otehr => other

platocrat
platocrat reviewed Mar 14, 2021
View reviewed changes
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_DepositedERC721.sol
*
* @param _tokenGateway Address of the corresponding gateway deployed to the other side
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newline

platocrat
platocrat reviewed Mar 14, 2021
View reviewed changes
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_DepositedERC721.sol
* param _to Address being withdrawn to
* param _tokenId Token being withdrawn
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newline

platocrat
platocrat reviewed Mar 14, 2021
View reviewed changes
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_DepositedERC721.sol
* dynamic, and the above public constant does not suffice.
*
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newline

Copy link
Contributor

@platocrat platocrat Mar 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

these extra lines are also in optimism's original Abs_X contracts too

platocrat
platocrat reviewed Mar 14, 2021
View reviewed changes
contracts/optimistic-ethereum/OVM/bridge/tokens/Abs_DepositedERC721.sol
/**
* @dev Overridable getter for the *Other side* gas limit of settling the withdrawal, in the case it may be
* dynamic, and the above public constant does not suffice.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

an extra line in dev comments?

azf20 added 2 commits March 14, 2021 16:53
@azf20
linting
a3c170e
@azf20
Updating DepositedERC721 Default Withdrawal Gas
49f0260 
- Necessary in order to work with primary contract on L2
@azf20
Copy link
Author

azf20 commented Mar 15, 2021

Had to increase Default Withdrawal Gas on DepositedERC721 for this to work with the origin contract on L2. Not sure what the ramifications of that might be, comments welcome.

@maurelian
Copy link
Collaborator

Hey thank you for this. I'll take a look this week!

wighawag
wighawag reviewed Mar 23, 2021
View reviewed changes
contracts/optimistic-ethereum/OVM/bridge/tokens/OVM_ERC721Gateway.sol
override
{
// Hold on to the newly deposited funds
IERC721(originalToken).transferFrom(
Copy link

@wighawag wighawag Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

would be great to support ERC721 onERC721Received callback so user do not need to perform an approval tx
this would require change in Abs_ERC721Gateway

Copy link
Author

Choose a reason for hiding this comment

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

that is a great call, will add in

@platocrat
Copy link
Contributor

Any updates on this?

@azf20 azf20 requested a review from smartcontracts as a code owner April 6, 2021 20:52
@azf20
Copy link
Author

azf20 commented Apr 6, 2021

I have added onERC721Received support to the gateway (cc @wighawag ), though I couldn't get the smocked Messenger to support it in the tests to check the message as in the other deposit tests. I have verified that it works as intended in an integration branch (https://github.com/austintgriffith/scaffold-eth/tree/oo-wip), so not sure if it is my user error, or something with smocked when it is called as the result of a callback:

const depositCallToMessenger =
        Mock__OVM_L1CrossDomainMessenger.smocked.sendMessage.calls[0]

in the new test throws with:
TypeError: Cannot read property 'map' of undefined

@platocrat & @maurelian not sure where this ended up (as I know there was a thread on bridging and the preferred approach there), happy to adjust this accordingly

@azf20
Copy link
Author

azf20 commented Apr 7, 2021

Fixed the smocked issue (with thanks to @smartcontracts see more here)

@anjubora anjubora mentioned this pull request Aug 17, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@platocrat platocrat platocrat left review comments

@wighawag wighawag wighawag left review comments

@ben-chain ben-chain Awaiting requested review from ben-chain ben-chain is a code owner

@maurelian maurelian Awaiting requested review from maurelian maurelian is a code owner

@smartcontracts smartcontracts Awaiting requested review from smartcontracts smartcontracts is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@azf20 @maurelian @platocrat @wighawag