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

Separate fee into home fee and foreign fee #151

Conversation

patitonar
Copy link
Contributor

Added the following methods:

  • setHomeFee
  • getHomeFee
  • setForeignFee
  • getForeignFee

I had to update calculateFee to receive a parameter to know which fee it should use.

If the Fee Manager is in manages-one-direction mode (see #148) the method setFee will proxy one of the methods setHomeFee and setForeignFee depending on the side where the contract is deployed. Similar is for getFee.

Regarding this comment, I removed setFee and getFee since I couldn't find a use case for them unless I'm missing something. On bridge-ui using getFeeManagerMode I think it will be enough to know which fee to use.

Closes #149

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.

We also told about the updates in the gas consumption document.

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

@akolotov Updated gas consumption document with latest 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.

Few questions appeared. Please share you view.

@@ -157,7 +159,7 @@ contract HomeBridgeErcToNative is EternalStorage, BasicBridge, BasicHomeBridge,

address feeManager = feeManagerContract();
if (feeManager != address(0)) {
uint256 fee = calculateFee(valueToMint, false, feeManager);
uint256 fee = calculateFee(valueToMint, false, feeManager, FOREIGN_FEE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a bit messed up. Let's clarify this together one more time:
Currently the examples of the .env file says that;

# Fee to be charged for each transfer on Home network
# Makes sense only when HOME_REWARDABLE=true
# e.g. 0.1% fee
HOME_TRANSACTIONS_FEE=0.001
# Fee to be charged for each transfer on Foreign network
# Makes sense only when FOREIGN_REWARDABLE=true
# e.g. 0.1% fee
FOREIGN_TRANSACTIONS_FEE=0.001

What does Fee to be charged for each transfer on Foreign network mean?
FOREIGN_FEE means fee that we take for every transaction that directed form the Foreign network to the Home network.
So, HOME_FEE means fee that we take for every transaction that directed form the Home network to the Foreign network
Is it your understanding also? If so, does it make sense to re-phrase the statements in the examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarification. Yes, that's my understanding too. I'll update documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed on b54cd53


function setFee(uint256 _fee) external onlyOwner {
_setFee(feeManagerContract(), _fee);
function setHomeFee(uint256 _fee) external onlyOwner {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to move these methods to the corresponding implementations of the bridges:

  • erc20_to_native/RewardableHomeBridgeErcToNative.sol:
    • setHomeFee
    • setForeignFee
    • getHomeFee
    • getForeignFee
  • native_to_erc20/RewardableHomeBridgeNativeToErc.sol
    • setForeignFee
    • getForeignFee
  • native_to_erc20/RewardableForeignBridgeNativeToErc.sol
    • setHomeFee
    • getHomeFee

By doing this we would:

  1. slightly reduce the code deployed.
  2. get rid of mess up especially in rhe case of native-to-erc mode.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on the changes, this would help on making code easier to follow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 78d6c3b

@ghost ghost removed the in progress label Feb 4, 2019
@patitonar
Copy link
Contributor Author

@akolotov Comments were addressed. Also fixed rewardableInitialize method to only initialize the correct fee parameter for each case and gas consumption docs

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 improving this!

@akolotov akolotov merged commit 0f15d7f into 119-Epic-rewards-for-bridge-validators Feb 5, 2019
@ghost ghost removed the review label Feb 5, 2019
@akolotov akolotov deleted the #149-separate-fee-home-foreign-transfers branch May 21, 2019 23:18
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.

2 participants