Create Governance based bridge contracts #3

rstormsf · 2018-03-02T23:47:21Z

Fixes:
omni/poa-bridge#8
omni/poa-bridge#15
omni/poa-bridge#17
omni/poa-bridge#18
omni/poa-bridge#20
omni/poa-bridge#9

Accepts ERC677 token
@akolotov

2. Add governance for validators 3. Make Home and Foreign Bridges operated from ValidatorsContract

akolotov · 2018-03-06T15:55:50Z

contracts/ForeignBridge.sol

-    /// Pending signatures and authorities who confirmed them
-    BridgeValidators public validatorsContract;
+    mapping (bytes32 => bool) tokenAddressApprovalSigns;
+    mapping (address => uint256) public numTokenAddressApprovalSigns;


It is not used any more. Could be removed.

akolotov · 2018-03-06T15:58:21Z

contracts/ForeignBridge.sol


    /// Collected signatures which should be relayed to home chain.
    event CollectedSignatures(address authorityResponsibleForRelay, bytes32 messageHash);

+    event TokenAddress(address token);


This is also not used. Please remove.

akolotov · 2018-03-06T16:04:45Z

contracts/ForeignBridge.sol

    }

+    function setGasLimitDepositRelay(uint256 gas) onlyValidator {


onlyValidator needs to be changed to onlyOwner

akolotov · 2018-03-06T16:05:01Z

contracts/ForeignBridge.sol

+        GasConsumptionLimitsUpdated(gasLimitDepositRelay, gasLimitWithdrawConfirm);
+    }
+
+    function setGasLimitWithdrawConfirm(uint256 gas) onlyValidator {


onlyValidator needs to be changed to onlyOwner

akolotov · 2018-03-06T16:07:15Z

contracts/ForeignBridge.sol

+        GasConsumptionLimitsUpdated(gasLimitDepositRelay, gasLimitWithdrawConfirm);
+    }
+
+    function setGasLimitWithdrawConfirm(uint256 gas) onlyValidator {


Let's consider to combine setGasLimitDepositRelay() and setGasLimitWithdrawConfirm in one method.

I don't think it's good idea

akolotov · 2018-03-06T16:20:22Z

contracts/ForeignBridge.sol

            Deposit(recipient, value);
        }
+        SignedForDeposit(msg.sender, transactionHash);


let's have:

SignedForDeposit(msg.sender, transactionHash); if (signed == validatorContract.requiredSignatures()) { // If the bridge contract does not own enough tokens to transfer // it will couse funds lock on the home side of the bridge erc677token.mint(recipient, value); Deposit(recipient, value); }

It will be more logical: SignedDeposit prior to Deposit

akolotov · 2018-03-06T16:34:22Z

contracts/ForeignBridge.sol


        // TODO: this may cause troubles if requiredSignatures len is changed
-        // if (signatures[hash].signed.length == requiredSignatures) {
+        if (signed == validatorContract.requiredSignatures()) {


Let's consider to have it more logical:

SignedForWithdraw(msg.sender, hash); if (signed == validatorContract.requiredSignatures()) { CollectedSignatures(msg.sender, hash); }

akolotov · 2018-03-06T16:48:40Z

contracts/ForeignBridge.sol

+        GasConsumptionLimitsUpdated(gasLimitDepositRelay, gasLimitWithdrawConfirm);
+    }
+
+    function deposit(address recipient, uint value, bytes32 transactionHash) public onlyValidator {


To not forget: create an issue to implement functionality that the bridge calls isValidator before sending a transaction.

omni/poa-bridge#28

akolotov · 2018-03-06T16:50:30Z

contracts/HomeBridge.sol

-    /// this shuts down attacks that exhaust authorities funds on home chain.
+contract HomeBridge is Validatable, BridgeDeploymentAddressStorage {
+    using SafeMath for uint256;
+    uint256 public gasLimitWithdrawRelay;
    uint256 public estimatedGasCostOfWithdraw;


It is not needed any more. Please remove.

akolotov · 2018-03-06T16:53:36Z

contracts/ForeignBridge.sol

        require(!deposits_signed[hash_sender]);
        deposits_signed[hash_sender] = true;

-        uint8 signed = num_deposits_signed[hash_msg] + 1;
+        uint signed = num_deposits_signed[hash_msg] + 1;


let's specify exact size of integer here: uint8 or uint256

akolotov · 2018-03-06T16:54:59Z

contracts/HomeBridge.sol

-
-        // The following two statements guard against reentry into this function.
-        // Duplicated withdraw or reentry.
+        uint256 homeGasPrice = Message.getHomeGasPrice(message);


Let's remove all homeGasPrice related logic and work with value only. Later we will remove it from the bridge binary and from ForeignBridge contract.

it does not make sense to cover expenses of bridge validators on Home side since Home is POA network and all transactions are "free" here.

it is not the only use case here, people can also withdraw if they provide the correct gasPrice value

akolotov · 2018-03-12T22:23:22Z

contracts/libraries/Helpers.sol

+        IBridgeValidators _validatorContract) internal view returns (bool) {
+        uint8 requiredSignatures = _validatorContract.requiredSignatures();
+        require(_vs.length <= requiredSignatures);


Должно быть

require(_vs.length >= requiredSignatures);

akolotov · 2018-03-12T22:29:55Z

contracts/libraries/Message.sol

+
+    function getHomeGasPrice(bytes message) internal pure returns (uint256) {


Не используется больше

akolotov · 2018-03-13T05:33:52Z

I've merged PR.

Does it make sense to put getCurrentDay(), setDailyLimit and withinLimit() in a separate contract which will be inherited both by HomeBridge and ForeignBridge. Seems that they have a common code part.

rstormsf added 2 commits March 3, 2018 00:38

1. Add ERC677 support

b034402

2. Add governance for validators 3. Make Home and Foreign Bridges operated from ValidatorsContract

merge

4d8c1e1

ghost added the in progress label Mar 2, 2018

rstormsf added 2 commits March 3, 2018 01:16

Provide flat contracts

7897740

Fixes omni#2

Fix omni#9. Extend tracebility of deposits transactions

cb33ddb

akolotov reviewed Mar 6, 2018

View reviewed changes

rstormsf added 4 commits March 11, 2018 12:37

Apply comments enhancements

54e34c5

Add daily Limits

a3e884c

Add hasEnoughSignatures back to working condition

48c782f

remove homeGasPrice from HomeBridge

7d1df24

akolotov reviewed Mar 12, 2018

View reviewed changes

Fix hasEnoughSignatures method.

f99f21a

akolotov merged commit d53ae35 into omni:master Mar 13, 2018

ghost removed the in progress label Mar 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Governance based bridge contracts #3

Create Governance based bridge contracts #3

rstormsf commented Mar 2, 2018 •

edited

Loading

akolotov Mar 6, 2018

akolotov Mar 6, 2018

akolotov Mar 6, 2018

akolotov Mar 6, 2018

akolotov Mar 6, 2018

rstormsf Mar 11, 2018

akolotov Mar 6, 2018

akolotov Mar 6, 2018

akolotov Mar 6, 2018

rstormsf Mar 11, 2018

akolotov Mar 6, 2018

akolotov Mar 6, 2018

akolotov Mar 6, 2018 •

edited

Loading

rstormsf Mar 11, 2018

akolotov Mar 12, 2018

rstormsf Mar 12, 2018 •

edited

Loading

akolotov Mar 12, 2018

akolotov Mar 12, 2018

akolotov commented Mar 13, 2018

		}

		function setGasLimitDepositRelay(uint256 gas) onlyValidator {


		function getHomeGasPrice(bytes message) internal pure returns (uint256) {

Create Governance based bridge contracts #3

Create Governance based bridge contracts #3

Conversation

rstormsf commented Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolotov Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstormsf Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolotov commented Mar 13, 2018

rstormsf commented Mar 2, 2018 •

edited

Loading

akolotov Mar 6, 2018 •

edited

Loading

rstormsf Mar 12, 2018 •

edited

Loading