omni / bridge-ui

UI for TokenBridge, an interoperability solution between Ethereum networks for native and ERC tokens
https://bridge.poa.net/
41 stars 51 forks source link

Update transfer alert to display fee information #182

Closed patitonar closed 5 years ago

patitonar commented 5 years ago

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[bot] commented 5 years ago

Deploy preview for kind-kilby-95344f processing.

Building with commit ead41b90831e435021d0b6f5b8c7d2b79f66a8cd

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

akolotov commented 5 years ago

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

patitonar commented 5 years ago

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

no-fees

akolotov commented 5 years ago

@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:

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 commented 5 years ago

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:

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

akolotov commented 5 years ago

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

I have created https://github.com/poanetwork/poa-bridge-contracts/issues/148 for this item.

akolotov commented 5 years ago

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 https://github.com/poanetwork/poa-bridge-contracts/issues/149 to address this.

patitonar commented 5 years ago

@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.

patitonar commented 5 years ago

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 https://github.com/poanetwork/poa-bridge-contracts/pull/143 is merged we should open a PR to change submodule branch to develop to maintain consistency