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 fee manager for Native-to-ERC that collects fees on home network #155

Merged

Conversation

patitonar
Copy link
Contributor

Closes #138

I had to make some changes on HomeBridgeNativeToErc.sol to make it work with the two fee manager approaches on this bridge mode.

Also I refactored how the .env variables HOME_REWARDABLE and FOREIGN_REWARDABLE works to be able to deploy the correct contracts on the deploy script.

After we agree on the changes, I'll update the gas consumption document as part of this PR.

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.

I think we can simplify things a bit.

If we look at the Home Bridge functionality from the Fee Management point of view it could be as following:

  1. the fallback function
  • change the value to transfer (if the Home fee is not zero. The Fee Manager is responsible for both directions)
  • keep the value to transfer (if the Home fee is zero. There is no fee manager or it is responsible for one direction)
  1. onSignaturesCollected
  • recover fee from the message and distribute it among the validators (if the Home fee is not zero. The Fee Manager is responsible for both directions)
  • nothing special (if the Home fee is zero. There is no fee manager or it is responsible for one direction)
    The same is applicable for the Foreign Bridge functionality:
  • onExecuteMessage
    • change the value received from the message, send the corresponding value of assets, distribute fee among the validators (if the Home fee is not zero. The Fee Manager is responsible for one directions)
    • keep the value received from the message unchanged and send the corresponding value of assets (if the Home fee is zero. There is no fee manager)

So, we don't need to introduce onRequestForSignature or onSignaturesCollected in the Fee Manager contract. We can implement similar we already done for the erc-to-native mode:

fallback, the HomeBridge contract

  uint256 valueToTransfer = msg.value;
  address feeManager = feeManagerContract();
  if (feeManager != address(0)) {
      uint256 fee = calculateFee(valueToTransfer, false, feeManager, HOME_FEE);
      valueToTransfer = valueToTransfer.sub(fee);
  }
  emit UserRequestForSignature(msg.sender, valueToTransfer);

It will work for both types of Fee Managers since the Home Fee is zero when there is no fee manager or it is responsible for one direction (called for onExecuteAffirmation only)

onSignaturesCollected, the HomeBridge contract

  address feeManager = feeManagerContract();
  if (feeManager != address(0)) {
      address recipient;
      uint256 amount;
      bytes32 txHash;
      address contractAddress;
      (recipient, amount, txHash, contractAddress) = Message.parseMessage(_message);
      uint256 fee = calculateFee(amount, true, feeManager, HOME_FEE);
      if (fee != 0) {
          distributeFeeFromSignatures(fee, feeManager);
      }
  }

By adding if (fee != 0) it will work for both types of Fee Managers since the Home Fee is zero when there is no fee manager or it is responsible for one direction

onExecuteMessage, the ForeignBridge contract

  uint256 valueToMint = _amount;
  address feeManager = feeManagerContract();
  if (feeManager != address(0)) {
      uint256 fee = calculateFee(valueToMint, false, feeManager, HOME_FEE);
      if (fee != 0) {
          distributeFeeFromSignatures(fee, feeManager);
          valueToMint = valueToMint.sub(fee);
      }
  }

By adding if (fee != 0) it will work in both scenarios as well. In the scenario when there is no Fee Manager the fee will be calculated as zero.

@ghost ghost added the in progress label Feb 25, 2019
@patitonar
Copy link
Contributor Author

Thanks for the suggestions, I applied the changes. If no more changes are needed, I'll update gas consumption document

…anager-POA-bridge

# Conflicts:
#	contracts/upgradeable_contracts/native_to_erc20/ForeignBridgeNativeToErc.sol
#	contracts/upgradeable_contracts/native_to_erc20/HomeBridgeNativeToErc.sol
@ghost ghost added the in progress label Apr 22, 2019
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 bringing this PR up to date! Let's merge it

@akolotov akolotov merged commit 19fa684 into 119-Epic-rewards-for-bridge-validators Apr 23, 2019
@ghost ghost removed the in progress label Apr 23, 2019
@akolotov akolotov deleted the #138-fee-manager-POA-bridge branch May 21, 2019 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants