Allow multiple bridges for token #387
Conversation
|
|
||
| contract ERC677BridgeToken is IBurnableMintableERC677Token, DetailedERC20, BurnableToken, MintableToken, Claimable { | ||
| address public bridgeContract; | ||
| address[] internal _bridgeContracts; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, I assume that setBridgeContracts is a rare operation.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, I will change the code making it similar to the linked-list approach you are proposing.
| function setBridgeContract(address _bridgeContract) external onlyOwner { | ||
| require(AddressUtils.isContract(_bridgeContract)); | ||
| bridgeContract = _bridgeContract; | ||
| function setBridgeContracts(address[] _contracts) external onlyOwner { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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].
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Renames
ERC677BridgeToken.setBridgeContracttosetBridgeContractsandERC677BridgeToken.bridgeContract()tobridgeContracts().Since the
ERC677BridgeTokenis not upgradable, we need to have an ability to set multiple addresses as there can be different bridges (in the future).