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

Add native to erc20 mediator on top of AMB #367

Merged
merged 35 commits into from
May 16, 2020
Merged

Conversation

patitonar
Copy link
Contributor

@patitonar patitonar commented Feb 13, 2020

Relates to #364, #365

@patitonar
Copy link
Contributor Author

I think this is ready for review, here are some useful comments:

Fee Manager

In the existing implementation of Native-to-erc we have two ways of collecting fees:

  • On both sides (current implementation in WETC bridge)
  • Only on home side for transfers in both directions

To collect the fees only on home side for both transfers we used onSignaturesCollected method which is called after all signatures are collected, so we could safely distribute fees there.

In a mediator implementation, we don't have that opportunity to call logic when the signatures are collected (which is OK since it is an internal operation of AMB) so we can't implement the logic to distribute fees in that way.

That's why the implementation for the fee manager only considers the way to distribute the fees on both sides when the message is correctly executed on the other network, which is the same way used in the WETC bridge. This also simplified the logic in the fee manager.

Another change is that I implemented two different contracts to be deployed as fee managers, one for home and one for foreign. In this case, each fee manager has the logic it only needs and also I think it helps to better understand its usage in the mediator contracts.

WETC specific

Regarding the specific usage of this mediator for WETC bridge, while there are no restrictions now from the network, there is a restriction from the current proxy contract that is being used.

The proxy contract being used is ClassicEternalStorageProxy. It has the restriction that for every call delegated to the implementation contract it will return a fixed-size value of 32 unless we specified the size for a method signature as we did for signature(bytes32,uint256) and message(bytes32).

In this case, it affects the method rewardAccounts() which returns the list of reward accounts that will get the fees that has a variable size. It doesn't affect the operations, fees are correctly distributed, but it is not possible to perform an external call if someone wants to know the list. That's why I implemented ClassicHomeAMBNativeToErc20 that maintains the size to be returned by the proxy so it will work correctly with the ClassicEternalStorageProxy proxy contract.

I tested both implementations in Sokol - Kovan with the existing AMB bridge:

  • Test with regular Home and Foreign versions
    [ Home ] Bridge Mediator: 0x21ec0Ccd8Cc8430dCb04AA4124AF7995E5Be50DF
    [ Home ] Fee Manager: 0x45220dec68F4733181D13e062dcF6106e4b0BdD8
    [ Foreign ] Bridge Mediator: 0x9cEB11d7a50F29eB1100D6EfBfbCa3586c27D03F
    [ Foreign ] Fee Manager: 0x604D0C32723E11A2E6b7D01717F3Afd730c76C4a
    [ Foreign ] ERC677 Token: 0x3459EDf5CEd322fC911bE03ACD3E177dD064d2Ba

  • Test with Classic version for Home and regular version for Foreign
    [ Home ] Bridge Mediator: 0x6c2bA57847e0a1B9eCC8292f6Dbe0776e4B9EAfc
    [ Home ] Fee Manager: 0x6d98Af026aBb1A2A8665f88C210f420A3615b3dA
    [ Foreign ] Bridge Mediator: 0x86c070D543A8aa4bCdEf1c78182F207dA55236Aa
    [ Foreign ] Fee Manager: 0x55Ff8C6D2aA87f291E1Ce74cbc74D775A7538E5e
    [ Foreign ] ERC677 Token: 0xD41BC633C05d4Db17122Cc9353DF31a0D51f99A2

After we agree on the changes, we should prepare to test the WETC bridge upgrade. The following are the possible steps for the upgrades and the methods to be called to migrate the storage, they were NOT tested.

Possible migration Steps
  1. An AMB bridge should be first deployed for ETC-ETH
  2. Deploy Home Fee manager contract
  3. Deploy Foreign Fee manager contract
  4. Update the upgrade methods with the missing contract addresses
  5. Deploy Classic home Mediator contract
  6. Deploy Foreign Mediator contract
  7. Stop bridge operations in both directions
  8. In Home call the proxy method upgradeToAndCall to call migrate method
  9. In Foreign call the proxy method upgradeToAndCall to call migrate method
  10. Resume bridge operations in both directions
Upgrade Method for home
/**
* @dev Method to migrate home WETC native-to-erc bridge to a mediator implementation on top of AMB
*/
function migrateToMediator() external {
    bytes32 REQUIRED_BLOCK_CONFIRMATIONS = 0x916daedf6915000ff68ced2f0b6773fe6f2582237f92c3c95bb4d79407230071; // keccak256(abi.encodePacked("requiredBlockConfirmations"))
    bytes32 GAS_PRICE = 0x55b3774520b5993024893d303890baa4e84b1244a43c60034d1ced2d3cf2b04b; // keccak256(abi.encodePacked("gasPrice"))
    bytes32 DEPLOYED_AT_BLOCK = 0xb120ceec05576ad0c710bc6e85f1768535e27554458f05dcbb5c65b8c7a749b0; // keccak256(abi.encodePacked("deployedAtBlock"))
    bytes32 HOME_FEE_STORAGE_KEY = 0xc3781f3cec62d28f56efe98358f59c2105504b194242dbcb2cc0806850c306e7; // keccak256(abi.encodePacked("homeFee"))
    bytes32 FOREIGN_FEE_STORAGE_KEY = 0x68c305f6c823f4d2fa4140f9cf28d32a1faccf9b8081ff1c2de11cf32c733efc; // keccak256(abi.encodePacked("foreignFee"))
    bytes32 VALIDATOR_CONTRACT = 0x5a74bb7e202fb8e4bf311841c7d64ec19df195fee77d7e7ae749b27921b6ddfe; // keccak256(abi.encodePacked("validatorContract"))

    bytes32 storageAddress = 0x131ab4848a6da904c5c205972a9dfe59f6d2afb8c9c3acd56915f89558369213; // keccak256(abi.encodePacked("migrationToMediator"))
    require(!boolStorage[storageAddress]);

    // Assign new AMB parameters
    _setBridgeContract(0x0000000000000000000000000000000000000000); // TODO set AMB bridge address when deployed on ETC
    _setMediatorContractOnOtherSide(0x0cB781EE62F815bdD9CD4c2210aE8600d43e7040);
    _setRequestGasLimit(0); // TODO define gas limit amount for foreign handleBridgedTokens method
    setNonce(keccak256(abi.encodePacked(address(this))));

    // Update fee parameters
    address feeManagerAddress = 0x0000000000000000000000000000000000000000; // TODO set new fee manager address
    addressStorage[FEE_MANAGER_CONTRACT] = feeManagerAddress;
    _setFee(feeManagerAddress, uintStorage[FOREIGN_FEE_STORAGE_KEY]);

    address[] memory list = new address[](1);
    list[0] = 0xCB576F21D18BD112D021813d4D6802d54Cacc0A0;
    _initializeRewardAccounts(list);

    // Free old storage
    delete addressStorage[VALIDATOR_CONTRACT];
    delete uintStorage[GAS_PRICE];
    delete uintStorage[DEPLOYED_AT_BLOCK];
    delete uintStorage[REQUIRED_BLOCK_CONFIRMATIONS];
    delete uintStorage[HOME_FEE_STORAGE_KEY];
    delete uintStorage[FOREIGN_FEE_STORAGE_KEY];

    // Free storage related to proxy size returns.
    delete uintStorage[0x5e16d82565fc7ee8775cc18db290ff4010745d3fd46274a7bc7ddbebb727fa54]; // keccak256(abi.encodePacked("dataSizes", bytes4(keccak256("signature(bytes32,uint256)"))))
    delete uintStorage[0x3b0a1ac531be1657049cf649eca2510ce9e3ef7df1be26d5c248fe8b298f4374]; // keccak256(abi.encodePacked("dataSizes", bytes4(keccak256("message(bytes32)"))))

    boolStorage[storageAddress] = true;
}
Upgrade Method for foreign
/**
* @dev Method to migrate foreign WETC native-to-erc bridge to a mediator implementation on top of AMB
*/
function migrateToMediator() external {
    bytes32 REQUIRED_BLOCK_CONFIRMATIONS = 0x916daedf6915000ff68ced2f0b6773fe6f2582237f92c3c95bb4d79407230071; // keccak256(abi.encodePacked("requiredBlockConfirmations"))
    bytes32 GAS_PRICE = 0x55b3774520b5993024893d303890baa4e84b1244a43c60034d1ced2d3cf2b04b; // keccak256(abi.encodePacked("gasPrice"))
    bytes32 DEPLOYED_AT_BLOCK = 0xb120ceec05576ad0c710bc6e85f1768535e27554458f05dcbb5c65b8c7a749b0; // keccak256(abi.encodePacked("deployedAtBlock"))
    bytes32 HOME_FEE_STORAGE_KEY = 0xc3781f3cec62d28f56efe98358f59c2105504b194242dbcb2cc0806850c306e7; // keccak256(abi.encodePacked("homeFee"))
    bytes32 FOREIGN_FEE_STORAGE_KEY = 0x68c305f6c823f4d2fa4140f9cf28d32a1faccf9b8081ff1c2de11cf32c733efc; // keccak256(abi.encodePacked("foreignFee"))
    bytes32 VALIDATOR_CONTRACT = 0x5a74bb7e202fb8e4bf311841c7d64ec19df195fee77d7e7ae749b27921b6ddfe; // keccak256(abi.encodePacked("validatorContract"))

    bytes32 storageAddress = 0x131ab4848a6da904c5c205972a9dfe59f6d2afb8c9c3acd56915f89558369213; // keccak256(abi.encodePacked("migrationToMediator"))
    require(!boolStorage[storageAddress]);

    // Assign new AMB parameters
    _setBridgeContract(0x0000000000000000000000000000000000000000); // TODO set AMB bridge address when deployed on ETH
    _setMediatorContractOnOtherSide(0x073081832b4ecdce79d4d6753565c85ba4b3bea9);
    _setRequestGasLimit(0); // TODO define gas limit amount for home handleBridgedTokens method
    setNonce(keccak256(abi.encodePacked(address(this))));

    // Update fee parameters
    address feeManagerAddress = 0x0000000000000000000000000000000000000000; // TODO set new fee manager address
    addressStorage[FEE_MANAGER_CONTRACT] = feeManagerAddress;
    _setFee(feeManagerAddress, uintStorage[HOME_FEE_STORAGE_KEY]);

    address[] memory list = new address[](1);
    list[0] = 0xCB576F21D18BD112D021813d4D6802d54Cacc0A0;
    _initializeRewardAccounts(list);

    // Free old storage
    delete addressStorage[VALIDATOR_CONTRACT];
    delete uintStorage[GAS_PRICE];
    delete uintStorage[DEPLOYED_AT_BLOCK];
    delete uintStorage[REQUIRED_BLOCK_CONFIRMATIONS];
    delete uintStorage[HOME_FEE_STORAGE_KEY];
    delete uintStorage[FOREIGN_FEE_STORAGE_KEY];

    boolStorage[storageAddress] = true;
}

@patitonar patitonar marked this pull request as ready for review February 18, 2020 14:57
@akolotov
Copy link
Collaborator

The proxy contract being used is ClassicEternalStorageProxy. It has the restriction that for every call delegated to the implementation contract it will return a fixed-size value of 32 unless we specified the size for a method signature as we did for signature(bytes32,uint256) and message(bytes32).

In this case, it affects the method rewardAccounts() which returns the list of reward accounts that will get the fees that has a variable size. It doesn't affect the operations, fees are correctly distributed, but it is not possible to perform an external call if someone wants to know the list. That's why I implemented ClassicHomeAMBNativeToErc20 that maintains the size to be returned by the proxy so it will work correctly with the ClassicEternalStorageProxy proxy contract.

Thanks for highlighting this. I need to think more if it does make sense still have separate versions of contracts.

Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

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

Thanks for highlighting this. I need to think more if it does make sense still have separate versions of contracts.

Could you investigate possibility and efforts to use the fee management contract as a separate contract rather than an extension of the mediator contract? In this case there will be no need to introduce a special handling for the rewardAccounts method.

The investigation should also take into account how many efforts could be required in the future to use the same set of contracts to migrate POA-POA20 bridge to a pair of mediators.

@patitonar
Copy link
Contributor Author

In order to use the fee manager contract as a separate contract, the mediator contract will only store the address of the fee manager contract, and so the fee and the reward accounts list will be stored in the fee manager contract.

In initialization terms, this means that a new proxy will be deployed along with the fee manager contract, and then it will be initialized defining an owner, the fee and the reward accounts.
In the mediator, when initializing, the fee manager proxy address will be used or set to zero in case this feature is not needed in the initialize method, so rewardableInitialize is no longer needed.

The fee manager will accept external calls to return the stored fee, calculate the fee amount of a value using the stored fee and return the list of reward accounts. Also, it will accept external calls from the owner to update the fee and add/remove accounts from the list.

For the fee calculation, the method with the logic will be defined in the fee manager contract, so the mediator will perform a regular call to get the calculated fee amount.

The fee distribution method needs to be executed in the context(with the state) of the mediator contract so it can access the tokens. I think the best option is to define the distributeFee method in the mediator contract and interact with the fee manager to iterate the reward accounts. One advantage of this is that the fee manager will be generic enough to only have one version instead of having one for home and one for foreign.

With this solution, we don't need a special mediator contract for the existing classic proxy because the rewardAccounts method will be in the fee manager contract using a regular proxy contract. Regarding the POA-POA20 bridge, the migration process and state variables updates require further analysis but so far I wasn't able to identify any restriction for which this solution won't work since the proxy contract used doesn't have any special handling and the rest of the code is upgradable.

Applying these changes doesn't require big efforts in the contracts since most of the methods are already implemented. First, we need to revert 8cc1e65. Then most of the work will be to move some methods, update the visibility/modifiers and update the initialization methods. The initialization of some of the unit tests will be affected and also the deployment script needs to be updated to deploy a new proxy and initialize the fee manager.

@akolotov
Copy link
Collaborator

Thanks for investigation! Below are my comments.

In initialisation terms, this means that a new proxy will be deployed along with the fee manager contract, and then it will be initialised defining an owner, the fee and the reward accounts.

What if we will not use the proxy template here at all and will refer to the implementation? What drawbacks of this solution do you see?

The fee distribution method needs to be executed in the context(with the state) of the mediator contract so it can access the tokens. I think the best option is to define the distributeFee method in the mediator contract and interact with the fee manager to iterate the reward accounts.

It does not look as a straightforward way. Can you consider another options which can be implemented? As soon as the mediator needs to operate with fees, it sends the corresponding number of tokens to the fee manager and calls a method (onTokenTransfer?) from the fee manager contract. The fee manager contract will go through the list of reward recipients and send tokens to them. It will allow us:

  • to use EOA instead of a manager
  • to not implement any logic to distribute fees and collect fees on the fee manager contract instead.

Both options are useful on early stage of migration to AMB.

This option could allow us still keep the logic of fee distribution on the fee manager side as so different actions with fees are possible. For example,

  • Instead of transfer tokens by the fee manager contract, the reward recipients could call a special method on the fee manager contract to withdraw the fee. It will allow to increase number of fee recipients and safe gas for the bridge validators.
  • Fee could be sent back to the bridge automatically as so the corresponding amount of tokens appeared on another side of the bridge.

@patitonar
Copy link
Contributor Author

Updated the implementation with the recommendation from the comments.
The fee manager is used as a separate contract and without a proxy. For fee distribution, the mediator sends/Mint the tokens to the fee manager and calls onTokenTransfer which will go through the list of reward recipients and send tokens to them.

It will allow us:

  • to use EOA instead of a manager
  • to not implement any logic to distribute fees and collect fees on the fee manager contract instead.

In the way it is implemented right now I don't think we will be able to use an EOA instead of a manager. The fee parameter and the logic to calculate the fee of a transfer are stored/handled in the fee manager.

Updated migration methods

Upgrade Method for home

Deploy HomeFeeManagerAMBNativeToErc20 in ETC and initialize it with the following parameters

  • 0x131c608EE32e18430eDb439296d1d40819b509E9 // Current upgradeabilityOwner of the bridge
  • 2000000000000000 // Fee value
  • [0xCB576F21D18BD112D021813d4D6802d54Cacc0A0] // Reward address value
/**
* @dev Method to migrate home WETC native-to-erc bridge to a mediator implementation on top of AMB
*/
function migrateToMediator() external {
    bytes32 REQUIRED_BLOCK_CONFIRMATIONS = 0x916daedf6915000ff68ced2f0b6773fe6f2582237f92c3c95bb4d79407230071; // keccak256(abi.encodePacked("requiredBlockConfirmations"))
    bytes32 GAS_PRICE = 0x55b3774520b5993024893d303890baa4e84b1244a43c60034d1ced2d3cf2b04b; // keccak256(abi.encodePacked("gasPrice"))
    bytes32 DEPLOYED_AT_BLOCK = 0xb120ceec05576ad0c710bc6e85f1768535e27554458f05dcbb5c65b8c7a749b0; // keccak256(abi.encodePacked("deployedAtBlock"))
    bytes32 HOME_FEE_STORAGE_KEY = 0xc3781f3cec62d28f56efe98358f59c2105504b194242dbcb2cc0806850c306e7; // keccak256(abi.encodePacked("homeFee"))
    bytes32 FOREIGN_FEE_STORAGE_KEY = 0x68c305f6c823f4d2fa4140f9cf28d32a1faccf9b8081ff1c2de11cf32c733efc; // keccak256(abi.encodePacked("foreignFee"))
    bytes32 VALIDATOR_CONTRACT = 0x5a74bb7e202fb8e4bf311841c7d64ec19df195fee77d7e7ae749b27921b6ddfe; // keccak256(abi.encodePacked("validatorContract"))

    bytes32 migrationToMediatorStorage = 0x131ab4848a6da904c5c205972a9dfe59f6d2afb8c9c3acd56915f89558369213; // keccak256(abi.encodePacked("migrationToMediator"))
    require(!boolStorage[migrationToMediatorStorage]);

    // Assign new AMB parameters
    _setBridgeContract(0x0000000000000000000000000000000000000000); // TODO set AMB bridge address when deployed on ETC
    _setMediatorContractOnOtherSide(0x0cB781EE62F815bdD9CD4c2210aE8600d43e7040);
    _setRequestGasLimit(0); // TODO define gas limit amount for foreign handleBridgedTokens method
    setNonce(keccak256(abi.encodePacked(address(this))));

    // Update fee manager
    addressStorage[FEE_MANAGER_CONTRACT] = 0x0000000000000000000000000000000000000000; // TODO set new fee manager address

    // Free old storage
    delete addressStorage[VALIDATOR_CONTRACT];
    delete uintStorage[GAS_PRICE];
    delete uintStorage[DEPLOYED_AT_BLOCK];
    delete uintStorage[REQUIRED_BLOCK_CONFIRMATIONS];
    delete uintStorage[HOME_FEE_STORAGE_KEY];
    delete uintStorage[FOREIGN_FEE_STORAGE_KEY];

    // Free storage related to proxy size returns.
    delete uintStorage[0x5e16d82565fc7ee8775cc18db290ff4010745d3fd46274a7bc7ddbebb727fa54]; // keccak256(abi.encodePacked("dataSizes", bytes4(keccak256("signature(bytes32,uint256)"))))
    delete uintStorage[0x3b0a1ac531be1657049cf649eca2510ce9e3ef7df1be26d5c248fe8b298f4374]; // keccak256(abi.encodePacked("dataSizes", bytes4(keccak256("message(bytes32)"))))

    boolStorage[migrationToMediatorStorage] = true;
}
Upgrade Method for foreign

Deploy ForeignFeeManagerAMBNativeToErc20 in ETH and initialize it with the following parameters

  • 0x131c608EE32e18430eDb439296d1d40819b509E9 // Current upgradeabilityOwner of the bridge
  • 2000000000000000 // Fee value
  • [0xCB576F21D18BD112D021813d4D6802d54Cacc0A0] // Reward address value
  • 0x86aaBCc646f290b9Fc9Bd05CE17C3858d1511Da1 // Token address
/**
* @dev Method to migrate foreign WETC native-to-erc bridge to a mediator implementation on top of AMB
*/
function migrateToMediator() external {
    bytes32 REQUIRED_BLOCK_CONFIRMATIONS = 0x916daedf6915000ff68ced2f0b6773fe6f2582237f92c3c95bb4d79407230071; // keccak256(abi.encodePacked("requiredBlockConfirmations"))
    bytes32 GAS_PRICE = 0x55b3774520b5993024893d303890baa4e84b1244a43c60034d1ced2d3cf2b04b; // keccak256(abi.encodePacked("gasPrice"))
    bytes32 DEPLOYED_AT_BLOCK = 0xb120ceec05576ad0c710bc6e85f1768535e27554458f05dcbb5c65b8c7a749b0; // keccak256(abi.encodePacked("deployedAtBlock"))
    bytes32 HOME_FEE_STORAGE_KEY = 0xc3781f3cec62d28f56efe98358f59c2105504b194242dbcb2cc0806850c306e7; // keccak256(abi.encodePacked("homeFee"))
    bytes32 FOREIGN_FEE_STORAGE_KEY = 0x68c305f6c823f4d2fa4140f9cf28d32a1faccf9b8081ff1c2de11cf32c733efc; // keccak256(abi.encodePacked("foreignFee"))
    bytes32 VALIDATOR_CONTRACT = 0x5a74bb7e202fb8e4bf311841c7d64ec19df195fee77d7e7ae749b27921b6ddfe; // keccak256(abi.encodePacked("validatorContract"))

    bytes32 migrationToMediatorStorage = 0x131ab4848a6da904c5c205972a9dfe59f6d2afb8c9c3acd56915f89558369213; // keccak256(abi.encodePacked("migrationToMediator"))
    require(!boolStorage[migrationToMediatorStorage]);

    // Assign new AMB parameters
    _setBridgeContract(0x0000000000000000000000000000000000000000); // TODO set AMB bridge address when deployed on ETH
    _setMediatorContractOnOtherSide(0x073081832b4ecdce79d4d6753565c85ba4b3bea9);
    _setRequestGasLimit(0); // TODO define gas limit amount for home handleBridgedTokens method
    setNonce(keccak256(abi.encodePacked(address(this))));

    // Update fee manager
    addressStorage[FEE_MANAGER_CONTRACT] = 0x0000000000000000000000000000000000000000; // TODO set new fee manager address

    // Free old storage
    delete addressStorage[VALIDATOR_CONTRACT];
    delete uintStorage[GAS_PRICE];
    delete uintStorage[DEPLOYED_AT_BLOCK];
    delete uintStorage[REQUIRED_BLOCK_CONFIRMATIONS];
    delete uintStorage[HOME_FEE_STORAGE_KEY];
    delete uintStorage[FOREIGN_FEE_STORAGE_KEY];

    boolStorage[migrationToMediatorStorage] = true;
}

@akolotov
Copy link
Collaborator

@k1rill-fedoseev please take a look. I know that the fee manager will be required for your activity related to implementation of a pair of mediators for the STAKE token.

# Conflicts:
#	deploy/README.md
#	deploy/deploy.js
#	deploy/src/loadContracts.js
#	deploy/src/loadEnv.js
#	flatten.sh
@patitonar
Copy link
Contributor Author

Added the event TokensBridged to be emitted when handleBridgedTokens is called in this pair of mediators. 6e611e6

I think we should also use it in the other mediators. If the definition looks good, I can apply the changes for the other mediators here too. (or probably is better in a new PR with only those changes?)

Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

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

(or probably is better in a new PR with only those changes?)

Yes, let's have them in a separate PR.

contracts/upgradeable_contracts/MediatorMessagesGuard.sol Outdated Show resolved Hide resolved
@patitonar
Copy link
Contributor Author

Yes, let's have them in a separate PR.

Created #409 to add the event to other mediators.

# Conflicts:
#	contracts/upgradeable_contracts/amb_erc677_to_erc677/BasicAMBErc677ToErc677.sol
@patitonar
Copy link
Contributor Author

Applied a similar fix from #408 to these mediators too 3c7a0fc

@akolotov akolotov merged commit 0a7b10e into develop May 16, 2020
@akolotov akolotov deleted the 365-amb-native-to-erc branch May 16, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants