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

Update transfer alert to display fee information #182

Merged
merged 7 commits into from
Feb 6, 2019
Merged

Conversation

patitonar
Copy link

Updated transfer alert to display fee information if contracts are using them. The values were updated to show the real value the user is going to receive on the other network.

bridgeui-fee-alert

From the contracts we can know if there are fees implemented on each sides, but there is no way to know which fee will be applied in the direction of the transfer the user is submitting. So for this implementation, we use home fee, and if not present, will use foreign fee to calculate values to display on transfer alert.

Let me know if you think we should add some text or display fee information in a different way.

It seems that there are some issues of caching on Travis CI, I had to disable caching of node_modules on travis configuration to make it work

closes #163

@netlify
Copy link

netlify bot commented Jan 31, 2019

Deploy preview for kind-kilby-95344f processing.

Building with commit ead41b9

https://app.netlify.com/sites/kind-kilby-95344f/deploys/5c59bf86112207000811b535

@akolotov
Copy link
Collaborator

@patitonar what the content of the window will be if we have no fees configured on the bride at all?

@patitonar
Copy link
Author

@akolotov If no fees are configured, the window will remain as it was before, without any mention to fees.

no-fees

@akolotov
Copy link
Collaborator

akolotov commented Jan 31, 2019

@akolotov If no fees are configured, the window will remain as it was before, without any mention to fees.

thanks!

From the contracts we can know if there are fees implemented on each sides, but there is no way to know which fee will be applied in the direction of the transfer the user is submitting. So for this implementation, we use home fee, and if not present, will use foreign fee to calculate values to display on transfer alert.

My understanding is that we have three main cases:

  1. No fees at all
  2. Fee Managers deployed on both sides of the bridge
  3. The Fee Manager on one side of the bridge covers fees in both direction.

So, does it make sense to introduce a method on the Fee Manager side that will inform about support of one of this case?
For example this method will return one of the values:

  • keccak256("manages-one-direction") - below as ONE
  • keccak256("manages-both-directions") - below as BOTH

And the first case will be handled by the check if the Fee Manager (below as FM) contract address is set in the bridge contract.

So, we will have the following combinations

Home Foreign Comment
No FM No FM No fee at all
ONE No FM Notify fee for tx from Foreign side only by taking the fee from Home side
No FM ONE Notify fee for tx from Home side only by taking the fee from Foreign side
ONE ONE Notify fee for tx in both direction side. The fee is taken from Home side if the transaction goes from Foreign. The fee is taken from Foreign side if the transaction goes from Home.
BOTH No FM Notify fee for tx in both direction side. The fee is taken from Home side for any direction.
No FM BOTH Not possible
ONE BOTH Not possible
BOTH ONE Possible but must be avoided.
BOTH BOTH Not possible

Moreover for the case when we have the Fee Manager on one side but working in both direction I would suggest to have to separate variables that keep the fee for different direction. We have them configured differently in the .env-file anyway.

What do you think?

@patitonar
Copy link
Author

I agree with the proposed changes, they will allow us to use the correct fee to display the information.

I'll work on the contracts to:

  • Introduce a method on the Fee Manager side that will inform about support of one or both directions.
  • For the case when we have the Fee Manager on one side but working in both direction separate variables that keep the fee for different direction.

After those changes are done. We should update this implementation to work as explained in your comment

@akolotov
Copy link
Collaborator

akolotov commented Jan 31, 2019

Introduce a method on the Fee Manager side that will inform about support of one or both directions.

I have created omni/tokenbridge-contracts#148 for this item.

@akolotov
Copy link
Collaborator

For the case when we have the Fee Manager on one side but working in both direction separate variables that keep the fee for different direction.

Created omni/tokenbridge-contracts#149 to address this.

@ghost ghost added the in progress label Feb 5, 2019
@ghost ghost removed the in progress label Feb 5, 2019
@patitonar
Copy link
Author

@akolotov Updated the code to use the new methods from contracts getFeeManagerMode, getHomeFee, getForeignFee and implemented the logic from the combinations explained on the previous comments. Also I added some unit tests to this logic.

After changes on contracts are merge to develop, we should update submodule.

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.

Is my understanding correct and if we merge in develop this as is the UI will be broken? If no, I am OK to merge this. Otherwise let's have a separate branch for this feature until we have the contracts merged to develop.

@patitonar
Copy link
Author

There is no issue on merging this to develop, UI will work correctly with contracts that doesn't have the new fee related methods (feeManagerContract, getFeeManagerMode, etc). To get the ABI for the new methods, submodule was updated to 119-Epic-rewards-for-bridge-validators branch. After omni/tokenbridge-contracts#143 is merged we should open a PR to change submodule branch to develop to maintain consistency

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