Skip to content
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

Lazy setting of bridged tokens metadata #43

Merged
merged 3 commits into from
Jun 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions contracts/mocks/NFTWithoutMetadata.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
pragma solidity 0.7.5;

import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import "../interfaces/IERC1155TokenReceiver.sol";

contract NFTWithoutMetadata {
function safeTransferFrom(
address _from,
address _to,
uint256 _id,
bytes memory _data
) external {
IERC721Receiver to = IERC721Receiver(_to);
require(to.onERC721Received(msg.sender, _from, _id, _data) == to.onERC721Received.selector);
}

function safeTransferFrom(
address _from,
address _to,
uint256 _id,
uint256 _amount,
bytes memory _data
) external {
IERC1155TokenReceiver to = IERC1155TokenReceiver(_to);
require(to.onERC1155Received(msg.sender, _from, _id, _amount, _data) == to.onERC1155Received.selector);
}

function balanceOf(address, uint256) external view returns (uint256) {
return 1000;
}

function ownerOf(uint256) external view returns (address) {
return msg.sender;
}
}
14 changes: 13 additions & 1 deletion contracts/tokens/ERC1155BridgeToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,18 @@ contract ERC1155BridgeToken is ERC1155, IBurnableMintableERC1155Token {
_burnBatch(msg.sender, _tokenIds, _values);
}

/**
* @dev Updated bridged token name/symbol parameters.
* Only bridge owner or bridge itself can call this method.
* @param _name new name parameter, will be saved as is, without additional suffixes like " from Mainnet".
* @param _symbol new symbol parameter.
*/
function setMetadata(string calldata _name, string calldata _symbol) external onlyOwner {
require(bytes(_name).length > 0 && bytes(_symbol).length > 0);
name = _name;
symbol = _symbol;
}

/**
* @dev Updates the bridge contract address.
* Can be called by bridge owner after token contract was instantiated.
Expand Down Expand Up @@ -170,6 +182,6 @@ contract ERC1155BridgeToken is ERC1155, IBurnableMintableERC1155Token {
uint64 patch
)
{
return (1, 1, 0);
return (1, 2, 0);
}
}
25 changes: 24 additions & 1 deletion contracts/tokens/ERC721BridgeToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,29 @@ contract ERC721BridgeToken is ERC721, IBurnableMintableERC721Token {
_burn(_tokenId);
}

// hack to access private fields in ERC721 contract
struct MetadataStorage {
string name;
string symbol;
}

/**
* @dev Updated bridged token name/symbol parameters.
* Only bridge owner or bridge itself can call this method.
* @param _name new name parameter, will be saved as is, without additional suffixes like " from Mainnet".
* @param _symbol new symbol parameter.
*/
function setMetadata(string calldata _name, string calldata _symbol) external onlyOwner {
require(bytes(_name).length > 0 && bytes(_symbol).length > 0);

MetadataStorage storage metadata;
assembly {
metadata.slot := 6
akolotov marked this conversation as resolved.
Show resolved Hide resolved
}
metadata.name = _name;
metadata.symbol = _symbol;
}

/**
* @dev Sets the base URI for all tokens.
* Can be called by bridge owner after token contract was instantiated.
Expand Down Expand Up @@ -118,6 +141,6 @@ contract ERC721BridgeToken is ERC721, IBurnableMintableERC721Token {
uint64 patch
)
{
return (1, 0, 1);
return (1, 1, 0);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,18 @@ abstract contract BasicNFTOmnibridge is
address bridgedToken = bridgedTokenAddress(_token);
if (bridgedToken == address(0)) {
if (bytes(_name).length == 0) {
require(bytes(_symbol).length > 0);
_name = _symbol;
} else if (bytes(_symbol).length == 0) {
_symbol = _name;
if (bytes(_symbol).length > 0) {
_name = _transformName(_symbol);
}
} else {
if (bytes(_symbol).length == 0) {
_symbol = _name;
}
_name = _transformName(_name);
}
bridgedToken = _values.length > 0
? address(new ERC1155TokenProxy(tokenImageERC1155(), _transformName(_name), _symbol, address(this)))
: address(new ERC721TokenProxy(tokenImageERC721(), _transformName(_name), _symbol, address(this)));
? address(new ERC1155TokenProxy(tokenImageERC1155(), _name, _symbol, address(this)))
: address(new ERC721TokenProxy(tokenImageERC721(), _name, _symbol, address(this)));
_setTokenAddressPair(_token, bridgedToken);
}

Expand Down Expand Up @@ -296,8 +300,6 @@ abstract contract BasicNFTOmnibridge is
string memory name = _readName(_token);
string memory symbol = _readSymbol(_token);

require(bytes(name).length > 0 || bytes(symbol).length > 0);

return
abi.encodeWithSelector(
this.deployAndHandleBridgedNFT.selector,
Expand Down
67 changes: 67 additions & 0 deletions test/omnibridge_nft/common.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const ERC721TokenProxy = artifacts.require('ERC721TokenProxy')
const ERC1155BridgeToken = artifacts.require('ERC1155BridgeToken')
const ERC1155TokenProxy = artifacts.require('ERC1155TokenProxy')
const ERC1155ReceiverMock = artifacts.require('ERC1155ReceiverMock')
const NFTWithoutMetadata = artifacts.require('NFTWithoutMetadata')
const NFTForwardingRulesManager = artifacts.require('NFTForwardingRulesManager')
const SelectorTokenGasLimitManager = artifacts.require('SelectorTokenGasLimitManager')
const OwnedUpgradeabilityProxy = artifacts.require('OwnedUpgradeabilityProxy')
Expand Down Expand Up @@ -608,6 +609,23 @@ function runTests(accounts, isHome) {
})
}

it('should relay tokens with missing metadata', async () => {
const token = await NFTWithoutMetadata.new()

const transfer = token.methods['safeTransferFrom(address,address,uint256,bytes)']
await transfer(owner, contract.address, 1, '0x').should.be.fulfilled

const events = await getEvents(ambBridgeContract, { event: 'MockedEvent' })
expect(events.length).to.be.equal(1)

const { data } = events[0].returnValues
expect(data.slice(0, 10)).to.be.equal(selectors.deployAndHandleBridgedNFT)
const args = web3.eth.abi.decodeParameters(['address', 'string', 'string'], data.slice(10))
expect(args[0]).to.be.equal(token.address)
expect(args[1]).to.be.equal('')
expect(args[2]).to.be.equal('')
})

it('should respect global bridging restrictions', async () => {
await contract.disableTokenBridging(ZERO_ADDRESS, true).should.be.fulfilled
for (const send of sendFunctions) {
Expand Down Expand Up @@ -1135,6 +1153,22 @@ function runTests(accounts, isHome) {
expect(await deployedToken.symbol()).to.be.equal('Test')
})

it('should use default name, which can be reset later', async () => {
const data = deployAndHandleBridgedERC721({ tokenId: 1, name: '', symbol: '' })

expect(await executeMessageCall(exampleMessageId, data)).to.be.equal(true)

const deployedToken = await ERC721BridgeToken.at(await contract.bridgedTokenAddress(otherSideToken1))
expect(await deployedToken.name()).to.be.equal('')
expect(await deployedToken.symbol()).to.be.equal('')

await deployedToken.setMetadata('newName', 'newSymbol', { from: user }).should.be.rejected
await deployedToken.setMetadata('newName', 'newSymbol', { from: owner }).should.be.fulfilled

expect(await deployedToken.name()).to.be.equal('newName')
expect(await deployedToken.symbol()).to.be.equal('newSymbol')
})

it('should not allow to operate when execution is disabled globally', async () => {
const data = deployAndHandleBridgedERC721({ tokenId: 1 })

Expand Down Expand Up @@ -1361,6 +1395,23 @@ function runTests(accounts, isHome) {
expect(await token.balanceOf(contract.address, tokenId2)).to.be.bignumber.equal('12')
})

it('should relay tokens with missing metadata', async () => {
const token = await NFTWithoutMetadata.new()

const transfer = token.methods['safeTransferFrom(address,address,uint256,uint256,bytes)']
await transfer(owner, contract.address, 1, 10, '0x').should.be.fulfilled

const events = await getEvents(ambBridgeContract, { event: 'MockedEvent' })
expect(events.length).to.be.equal(1)

const { data } = events[0].returnValues
expect(data.slice(0, 10)).to.be.equal(selectors.deployAndHandleBridgedNFT)
const args = web3.eth.abi.decodeParameters(['address', 'string', 'string'], data.slice(10))
expect(args[0]).to.be.equal(token.address)
expect(args[1]).to.be.equal('')
expect(args[2]).to.be.equal('')
})

describe('fixFailedMessage', () => {
it(`should fix locked tokens`, async () => {
const tokenId1 = await mintNewERC1155(20)
Expand Down Expand Up @@ -1692,6 +1743,22 @@ function runTests(accounts, isHome) {
const event = await getEvents(contract, { event: 'TokensBridged' })
expect(event.length).to.be.equal(2)
})

it('should use default name, which can be reset later', async () => {
const data = deployAndHandleBridgedERC1155({ tokenIds: [1, 2], values: [1, 3], name: '', symbol: '' })
akolotov marked this conversation as resolved.
Show resolved Hide resolved

expect(await executeMessageCall(exampleMessageId, data)).to.be.equal(true)

const deployedToken = await ERC1155BridgeToken.at(await contract.bridgedTokenAddress(otherSideToken1))
expect(await deployedToken.name()).to.be.equal('')
expect(await deployedToken.symbol()).to.be.equal('')

await deployedToken.setMetadata('newName', 'newSymbol', { from: user }).should.be.rejected
await deployedToken.setMetadata('newName', 'newSymbol', { from: owner }).should.be.fulfilled

expect(await deployedToken.name()).to.be.equal('newName')
expect(await deployedToken.symbol()).to.be.equal('newSymbol')
})
})

describe('handleBridgedNFT', () => {
Expand Down