Skip to content

Allow multiple bridges for token #387

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.

Sign up for GitHub

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

Merged
akolotov merged 3 commits into from
Mar 23, 2020
Merged

Allow multiple bridges for token #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c64eafb
Allow multiple bridges for token
varasev Mar 20, 2020
72c62d3
Remove excess line
varasev Mar 20, 2020
ae66b52
Linter fix
varasev Mar 20, 2020
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
26 changes: 21 additions & 5 deletions contracts/ERC677BridgeToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,29 @@ import "./interfaces/IBurnableMintableERC677Token.sol";
import "./upgradeable_contracts/Claimable.sol";

contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, BurnableToken, MintableToken, Claimable {
address public bridgeContract;
address[] internal _bridgeContracts;
Copy link
Collaborator

@akolotov akolotov Mar 23, 2020

Choose a reason for hiding this comment

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

well... we can reduce gas consumption if the approach similar to storing the bridge validators will be applied.

It will allow to use the same structure to keep both the list of bridges and quick access to check the allowance to use them.

Copy link
Contributor Author

@varasev varasev Mar 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Although I don't expect to have a lot of bridges, I agree with you that it should use the linked-list approach. But since there is MAX_VALIDATORS limit of 50 items (because of block gas limit, I guess) and _removeValidator requires looping through the list, I'm not sure this approach would significantly win in gas. However, it could win in storage space (because the list approach uses one uint256 field and one mapping against my approach which uses an array and a mapping), but in case of a small number of bridges it doesn't matter.

Copy link
Contributor Author

@varasev varasev Mar 23, 2020

Choose a reason for hiding this comment

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

Also, I assume that setBridgeContracts is a rare operation.

Copy link
Collaborator

@akolotov akolotov Mar 23, 2020

Choose a reason for hiding this comment

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

In fact, any array in Solidity requires N+1 storage slots: N storage for the N array elements and 1 element to store the array index/length. So, storing the one bridge address in the linked list has the same efficiency as your approach, storing two bridge addresses in the linked list will consume less gas (by 1 storage slot), storing 3 bridge addresses -- by 2 storage slots etc. Here we are not considering the case that a transaction to send an array with one element requires more bytes since the array is RLP-encoded, so, even to decode the input data EVM will spend more resources.

Copy link
Collaborator

@akolotov akolotov Mar 23, 2020

Choose a reason for hiding this comment

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

BTW, 50 is a limitation which was asked to be added on one of the previous security audit. Bearing in the mind the same idea, it makes sense to limit the size of the bridge addresses list.

Copy link
Contributor Author

@varasev varasev Mar 23, 2020

Choose a reason for hiding this comment

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

Ok, I will change the code making it similar to the linked-list approach you are proposing.

mapping(address => bool) internal _isBridgeContract;

event ContractFallbackCallFailed(address from, address to, uint256 value);

constructor(string _name, string _symbol, uint8 _decimals) public DetailedERC20(_name, _symbol, _decimals) {
// solhint-disable-previous-line no-empty-blocks
}

function setBridgeContract(address _bridgeContract) external onlyOwner {
require(AddressUtils.isContract(_bridgeContract));
bridgeContract = _bridgeContract;
function setBridgeContracts(address[] _contracts) external onlyOwner {
Copy link
Collaborator

@akolotov akolotov Mar 23, 2020

Choose a reason for hiding this comment

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

Should we consider a case when the bridge is required to be removed from the list. E.g. we shut down it by some reasons?

Copy link
Contributor Author

@varasev varasev Mar 23, 2020

Choose a reason for hiding this comment

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

The new setBridgeContracts function assumes that we just rewrite the list of bridges.

For example, we have three bridges: [0x123, 0x456, 0x789]. And then we want to remove the second one (0x456). We just pass the new array to the setBridgeContracts: [0x123, 0x789].

Copy link
Collaborator

@akolotov akolotov Mar 23, 2020

Choose a reason for hiding this comment

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

Ok. Got you. Does it look like that we need to delete element here in order to get gas refund?

for (i = 0; i < _bridgeContracts.length; i++) {
    _isBridgeContract[_bridgeContracts[i]] = false;
}

Imagine that we are completely overwriting the array and replacing [0x123] by [0x321]? even for the case when the array [0x567, 0x789] is overwritten by [0x345, 0x789] it is still looks attractive.

Copy link
Contributor Author

@varasev varasev Mar 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean using delete _isBridgeContract[_bridgeContracts[i]]; instead of _isBridgeContract[_bridgeContracts[i]] = false;? I guess, these two statements are the same: https://solidity.readthedocs.io/en/v0.4.24/types.html#delete

require(_contracts.length > 0);
uint256 i;

for (i = 0; i < _bridgeContracts.length; i++) {
_isBridgeContract[_bridgeContracts[i]] = false;
}

_bridgeContracts = _contracts;

for (i = 0; i < _contracts.length; i++) {
require(AddressUtils.isContract(_contracts[i]));
_isBridgeContract[_contracts[i]] = true;
}
}

modifier validRecipient(address _recipient) {
Expand All @@ -27,6 +39,10 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna
_;
}

function bridgeContracts() external view returns (address[]) {
return _bridgeContracts;
}

function transferAndCall(address _to, uint256 _value, bytes _data) external validRecipient(_to) returns (bool) {
require(superTransfer(_to, _value));
emit Transfer(msg.sender, _to, _value, _data);
Expand Down Expand Up @@ -59,7 +75,7 @@ contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, Burna

function callAfterTransfer(address _from, address _to, uint256 _value) internal {
if (AddressUtils.isContract(_to) && !contractFallback(_from, _to, _value, new bytes(0))) {
require(_to != bridgeContract);
require(!_isBridgeContract[_to]);
emit ContractFallbackCallFailed(_from, _to, _value);
}
}
Expand Down
16 changes: 13 additions & 3 deletions deploy/src/deploymentUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ async function transferOwnership({ contract, newOwner, nonce, url }) {
}

async function setBridgeContract({ contract, bridgeAddress, nonce, url }) {
const data = await contract.methods.setBridgeContract(bridgeAddress).encodeABI()
const data = await contract.methods.setBridgeContracts([bridgeAddress]).encodeABI()
const sendTx = getSendTxMethod(url)
const result = await sendTx({
data,
Expand All @@ -257,7 +257,7 @@ async function setBridgeContract({ contract, bridgeAddress, nonce, url }) {
if (result.status) {
assert.strictEqual(Web3Utils.hexToNumber(result.status), 1, 'Transaction Failed')
} else {
await assertStateWithRetry(contract.methods.bridgeContract().call, bridgeAddress)
await assertStateWithRetry(contract.methods.bridgeContracts().call, [bridgeAddress])
}
}

Expand Down Expand Up @@ -301,7 +301,9 @@ async function initializeValidators({
async function assertStateWithRetry(fn, expected) {
return promiseRetry(async retry => {
const value = await fn()
if (value !== expected && value.toString() !== expected) {
if (Array.isArray(expected) && !value.equalsIgnoreCase(expected)) {
retry(`Transaction Failed. Expected: ${expected.toString()} Actual: ${value.toString()}`)
} else if (value !== expected && value.toString() !== expected) {
retry(`Transaction Failed. Expected: ${expected} Actual: ${value}`)
}
})
Expand All @@ -316,6 +318,14 @@ async function isContract(web3, address) {
return code !== '0x' && code !== '0x0'
}

Array.prototype.equalsIgnoreCase = function(array) {
return this.length == array.length && this.every((this_v, i) => { return this_v.equalsIgnoreCase(array[i]) });
}

String.prototype.equalsIgnoreCase = function(compareString) {
return this.toLowerCase() === compareString.toLowerCase();
};

module.exports = {
deployContract,
sendRawTxHome,
Expand Down
4 changes: 2 additions & 2 deletions deploy/src/erc_to_erc/preDeploy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ async function preDeploy() {
if (ERC20_EXTENDED_BY_ERC677) {
const tokenContract = new web3Foreign.eth.Contract(abi, ERC20_TOKEN_ADDRESS)
try {
await tokenContract.methods.bridgeContract().call()
await tokenContract.methods.bridgeContracts().call()
} catch (e) {
throw new Error(
`ERC20_EXTENDED_BY_ERC677 is set to TRUE but bridgeContract method was not found on ERC677 token.`
`ERC20_EXTENDED_BY_ERC677 is set to TRUE but bridgeContracts method was not found on ERC677 token.`
)
}
}
Expand Down
2 changes: 1 addition & 1 deletion docs/ERC-TO-ERC.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ EternalStorageProxy|deployment|378510|378510|378510
HomeBridgeErcToErc|deployment|3528509|3528509|3528509
EternalStorageProxy|upgradeTo|35871|30924|30913
ERC677BridgeToken|deployment|1498202|1499226|1498829
ERC677BridgeToken|setBridgeContract|29432|44432|39432
ERC677BridgeToken|setBridgeContracts|47552|86474|77323
ERC677BridgeToken|transferOwnership|30860|30924|30913
HomeBridgeErcToErc|initialize|212299|213195|213003
EternalStorageProxy|transferProxyOwnership|30653|30653|30653
Expand Down
4 changes: 2 additions & 2 deletions docs/NATIVE-TO-ERC-WITH-REWARD.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ ForeignBridgeNativeToErc|deployment|3931739|3931739|3931739
EternalStorageProxy|upgradeTo|35871|30924|30913
FeeManagerNativeToErc|deployment|1079956|1079956|1079956
ForeignBridgeNativeToErc|rewardableInitialize|329022|329086|329077
ERC677BridgeToken|setBridgeContract|29432|44432|39432
ERC677BridgeToken|setBridgeContracts|47552|86474|77323
ERC677BridgeToken|transferOwnership|30860|30924|30913
EternalStorageProxy|transferProxyOwnership|30653|30653|30653
Total| |9573114|9799953|9689237
Expand Down Expand Up @@ -92,7 +92,7 @@ EternalStorageProxy|deployment|378510|378510|378510
ForeignBridgeNativeToErc|deployment|3931739|3931739|3931739
EternalStorageProxy|upgradeTo|35871|30924|30913
ForeignBridgeNativeToErc|initialize|281275|281339|281328
ERC677BridgeToken|setBridgeContract|29432|44432|39432
ERC677BridgeToken|setBridgeContracts|47552|86474|77323
ERC677BridgeToken|transferOwnership|30860|30924|30913
EternalStorageProxy|transferProxyOwnership|30653|30653|30653
Total| |8189163|8291266|8250125
Expand Down
2 changes: 1 addition & 1 deletion docs/NATIVE-TO-ERC.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ EternalStorageProxy|deployment|378510|378510|378510
ForeignBridgeNativeToErc|deployment|3931739|3931739|3931739
EternalStorageProxy|upgradeTo|35871|30924|30913
ForeignBridgeNativeToErc|initialize|281275|281339|281328
ERC677BridgeToken|setBridgeContract|29432|44432|39432
ERC677BridgeToken|setBridgeContracts|47552|86474|77323
ERC677BridgeToken|transferOwnership|30860|30924|30913
EternalStorageProxy|transferProxyOwnership|30653|30653|30653
Total| |8189163|8291266|8250125
Expand Down
2 changes: 1 addition & 1 deletion test/amb_erc677_to_erc677/AMBErc677ToErc677Behavior.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ function shouldBehaveLikeBasicAMBErc677ToErc677(otherSideMediatorContract, accou
// Given
const erc677Token = await ERC677BridgeToken.new('test', 'TST', 18)
await erc677Token.mint(user, twoEthers, { from: owner }).should.be.fulfilled
await erc677Token.setBridgeContract(contract.address, { from: owner }).should.be.fulfilled
await erc677Token.setBridgeContracts([contract.address], { from: owner }).should.be.fulfilled
await erc677Token.transferOwnership(contract.address, { from: owner }).should.be.fulfilled

contract = this.bridge
Expand Down
48 changes: 24 additions & 24 deletions test/poa20_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,32 +46,32 @@ async function testERC677BridgeToken(accounts, rewardable) {
describe('#bridgeContract', async () => {
it('can set bridge contract', async () => {
const homeErcToErcContract = await HomeErcToErcBridge.new()
;(await token.bridgeContract()).should.be.equal(ZERO_ADDRESS)
;(await token.bridgeContracts()).length.should.be.equal(0)

await token.setBridgeContract(homeErcToErcContract.address).should.be.fulfilled
;(await token.bridgeContract()).should.be.equal(homeErcToErcContract.address)
await token.setBridgeContracts([homeErcToErcContract.address]).should.be.fulfilled
;(await token.bridgeContracts()).should.be.deep.equal([homeErcToErcContract.address])
})

it('only owner can set bridge contract', async () => {
const homeErcToErcContract = await HomeErcToErcBridge.new()
;(await token.bridgeContract()).should.be.equal(ZERO_ADDRESS)
;(await token.bridgeContracts()).length.should.be.equal(0)

await token.setBridgeContract(homeErcToErcContract.address, { from: user }).should.be.rejectedWith(ERROR_MSG)
;(await token.bridgeContract()).should.be.equal(ZERO_ADDRESS)
await token.setBridgeContracts([homeErcToErcContract.address], { from: user }).should.be.rejectedWith(ERROR_MSG)
;(await token.bridgeContracts()).length.should.be.equal(0)

await token.setBridgeContract(homeErcToErcContract.address, { from: owner }).should.be.fulfilled
;(await token.bridgeContract()).should.be.equal(homeErcToErcContract.address)
await token.setBridgeContracts([homeErcToErcContract.address], { from: owner }).should.be.fulfilled
;(await token.bridgeContracts()).should.be.deep.equal([homeErcToErcContract.address])
})

it('fail to set invalid bridge contract address', async () => {
const invalidContractAddress = '0xaaB52d66283F7A1D5978bcFcB55721ACB467384b'
;(await token.bridgeContract()).should.be.equal(ZERO_ADDRESS)
;(await token.bridgeContracts()).length.should.be.equal(0)

await token.setBridgeContract(invalidContractAddress).should.be.rejectedWith(ERROR_MSG)
;(await token.bridgeContract()).should.be.equal(ZERO_ADDRESS)
await token.setBridgeContracts([invalidContractAddress]).should.be.rejectedWith(ERROR_MSG)
;(await token.bridgeContracts()).length.should.be.equal(0)

await token.setBridgeContract(ZERO_ADDRESS).should.be.rejectedWith(ERROR_MSG)
;(await token.bridgeContract()).should.be.equal(ZERO_ADDRESS)
await token.setBridgeContracts([ZERO_ADDRESS]).should.be.rejectedWith(ERROR_MSG)
;(await token.bridgeContracts()).length.should.be.equal(0)
})
})

Expand Down Expand Up @@ -265,7 +265,7 @@ async function testERC677BridgeToken(accounts, rewardable) {
})

it('sends tokens to bridge contract', async () => {
await token.setBridgeContract(homeErcToErcContract.address).should.be.fulfilled
await token.setBridgeContracts([homeErcToErcContract.address]).should.be.fulfilled
await token.mint(user, oneEther, { from: owner }).should.be.fulfilled

const result = await token.transfer(homeErcToErcContract.address, minPerTx, { from: user }).should.be.fulfilled
Expand All @@ -275,7 +275,7 @@ async function testERC677BridgeToken(accounts, rewardable) {
value: minPerTx
})

await token.setBridgeContract(foreignNativeToErcBridge.address).should.be.fulfilled
await token.setBridgeContracts([foreignNativeToErcBridge.address]).should.be.fulfilled
const result2 = await token.transfer(foreignNativeToErcBridge.address, minPerTx, {
from: user
}).should.be.fulfilled
Expand All @@ -287,7 +287,7 @@ async function testERC677BridgeToken(accounts, rewardable) {
})

it('sends tokens to contract that does not contains onTokenTransfer method', async () => {
await token.setBridgeContract(homeErcToErcContract.address).should.be.fulfilled
await token.setBridgeContracts([homeErcToErcContract.address]).should.be.fulfilled
await token.mint(user, oneEther, { from: owner }).should.be.fulfilled

const result = await token.transfer(validatorContract.address, minPerTx, { from: user }).should.be.fulfilled
Expand All @@ -307,10 +307,10 @@ async function testERC677BridgeToken(accounts, rewardable) {
const lessThanMin = ether('0.0001')
await token.mint(user, oneEther, { from: owner }).should.be.fulfilled

await token.setBridgeContract(homeErcToErcContract.address).should.be.fulfilled
await token.setBridgeContracts([homeErcToErcContract.address]).should.be.fulfilled
await token.transfer(homeErcToErcContract.address, lessThanMin, { from: user }).should.be.rejectedWith(ERROR_MSG)

await token.setBridgeContract(foreignNativeToErcBridge.address).should.be.fulfilled
await token.setBridgeContracts([foreignNativeToErcBridge.address]).should.be.fulfilled
await token
.transfer(foreignNativeToErcBridge.address, lessThanMin, { from: user })
.should.be.rejectedWith(ERROR_MSG)
Expand Down Expand Up @@ -343,7 +343,7 @@ async function testERC677BridgeToken(accounts, rewardable) {
const amount = ether('1')
const user2 = accounts[2]

await token.setBridgeContract(receiver.address).should.be.fulfilled
await token.setBridgeContracts([receiver.address]).should.be.fulfilled

expect(await receiver.from()).to.be.equal(ZERO_ADDRESS)
expect(await receiver.value()).to.be.bignumber.equal(ZERO)
Expand Down Expand Up @@ -480,7 +480,7 @@ async function testERC677BridgeToken(accounts, rewardable) {
})

it('sends tokens to bridge contract', async () => {
await token.setBridgeContract(homeErcToErcContract.address).should.be.fulfilled
await token.setBridgeContracts([homeErcToErcContract.address]).should.be.fulfilled
await token.mint(user, oneEther, { from: owner }).should.be.fulfilled

const result = await token.transferAndCall(homeErcToErcContract.address, minPerTx, '0x', {
Expand All @@ -492,7 +492,7 @@ async function testERC677BridgeToken(accounts, rewardable) {
value: minPerTx
})

await token.setBridgeContract(foreignNativeToErcBridge.address).should.be.fulfilled
await token.setBridgeContracts([foreignNativeToErcBridge.address]).should.be.fulfilled
const result2 = await token.transferAndCall(foreignNativeToErcBridge.address, minPerTx, '0x', { from: user })
.should.be.fulfilled
expectEventInLogs(result2.logs, 'Transfer', {
Expand All @@ -503,7 +503,7 @@ async function testERC677BridgeToken(accounts, rewardable) {
})

it('fail to sends tokens to contract that does not contains onTokenTransfer method', async () => {
await token.setBridgeContract(homeErcToErcContract.address).should.be.fulfilled
await token.setBridgeContracts([homeErcToErcContract.address]).should.be.fulfilled
await token.mint(user, oneEther, { from: owner }).should.be.fulfilled

await token
Expand All @@ -515,12 +515,12 @@ async function testERC677BridgeToken(accounts, rewardable) {
const lessThanMin = ether('0.0001')
await token.mint(user, oneEther, { from: owner }).should.be.fulfilled

await token.setBridgeContract(homeErcToErcContract.address).should.be.fulfilled
await token.setBridgeContracts([homeErcToErcContract.address]).should.be.fulfilled
await token
.transferAndCall(homeErcToErcContract.address, lessThanMin, '0x', { from: user })
.should.be.rejectedWith(ERROR_MSG)

await token.setBridgeContract(foreignNativeToErcBridge.address).should.be.fulfilled
await token.setBridgeContracts([foreignNativeToErcBridge.address]).should.be.fulfilled
await token
.transferAndCall(foreignNativeToErcBridge.address, lessThanMin, '0x', { from: user })
.should.be.rejectedWith(ERROR_MSG)
Expand Down