Add Burner Wallet Plugin for WETC Bridge

[patitonar]

Relates to omni/tokenbridge-contracts#365

This work was originally submitted on a separate repo and now integrated into the monorepo. Also, the burner-wallet dependencies were updated to the latest version.

The plugin adds the following definitions to be used by the existing Exchange plugin:

Assets:

• ETC
• ERC677
• WETC

Gateway:

• EtcGateway

Pair:

• Bridge
• WETCBridge

Project structure

• tokenbridge-plugin plugin definition
• local-wallet Burner wallet using the plugin with a Native - ERC20 bridge deployed in Sokol - Kovan on top of AMB.
• basic-wallet Burner wallet using the plugin with the existing WETC Bridge in mainnet and Classic.

I tested the plugin in Sokol - Kovan and also with the WETC Bridge in mainnet and Classic and it worked correctly.

Here are some notes from my initial testing related more to the Burner Wallet user experience rather than the plugin itself:

• When instead of using the PK, Metamask or Nifty Wallet is used, the popup to sign the transaction is not triggered when using different networks than the required for the exchange transaction and no feedback is provided. I guess this behavior is Ok but giving some feedback to the user that it is in an incorrect network would help. This is related to the official exchange plugin. After upgrading the exchange version, now a message is displayed when the wallet is not on the correct network.
• When using Nifty wallet, the popup to sign the transaction is not triggered for the exchange transaction even when it is set to the correct network. It works correctly when using Metamask. I think it is probably related to the official metamask-plugin. This was fixed with the latest version of Nifty wallet and now works correctly.
• For all cases, after the exchange transaction is performed and mined there is no feedback to the user. You have just to wait until the balances are updated. In this case of bridges, you can see one balance reduced and after a couple of seconds, the other balance is updated with the bridged tokens. This is related to the official exchange plugin, I guess a possible improvement to propose would be to allow Exchange Pairs to set a feedback message after the execution so the user can be informed on the delay due to the bridge operations.

[akolotov]

I have reviewed your approach and few ideas came to my mind how we can improve it.

1. Do you think it makes sense to introduce a generic gateway (TokenBridgeGateway?) similar to InfuraGateway. This gateway will supply Web3 providers for our chains: ETC, Sokol, POA. If my understanding correct, InfuraGateway make it happens by using chain id, so, we can do by the same manner.

2. What if we introduce a new asset sPOA?

3. All assets, the gateway and Bridge.ts will be placed to the tokenbridge-burnerwallet directory. It will allow us for every new plugin (in fact, a pair) create a new directory like tokenbridge-burnerwallet-wetcbridge which will have only two files: the bridge definition (like WETCBridge.ts) and index.ts. index.ts will contain only one line in this case. E.g. for tokenbridge-burnerwallet-wetcbridge it could be:

   export { default as WETCBridge } from './WETCBridge'
   

4. We could consider to rename the directories basic-wallet and local-wallet to staging and testing correspondingly (it will also require two docker files: Dockerfile.staging and Dockerfile.testing). In staging, we will extend the list of assets and pairs every time when an official plugin is prepared. In testing, the code will check REACT_APP_MODE to build the corresponding assets and the pair. For example, the code could look like:

   if ( process.env.REACT_APP_MODE === 'AMB_NATIVE_TO_ERC677' ) {
     const assetAtHome = new sPOA({id: 'assetAtHome'})
   
     const assetAtForeign = new ERC677Asset({
       id: 'assetAtForeign',
       name: process.env.REACT_APP_FOREIGN_TOKEN_NAME,
       network: process.env.REACT_APP_FOREIGN_NETWORK,
       address: process.env.REACT_APP_FOREIGN_TOKEN_ADDRESS
     })
   } else if ( process.env.REACT_APP_MODE === 'AMB_ERC677_TO_ERC677' ) {
     const assetAtHome = new ERC677Asset({
       id: 'assetAtHome',
       name: process.env.REACT_APP_HOME_TOKEN_NAME,
       network: process.env.REACT_APP_HOME_NETWORK,
       address: process.env.REACT_APP_HOME_TOKEN_ADDRESS
     })
   
     const assetAtForeign = new ERC677Asset({
       id: 'assetAtForeign',
       name: process.env.REACT_APP_FOREIGN_TOKEN_NAME,
       network: process.env.REACT_APP_FOREIGN_NETWORK,
       address: process.env.REACT_APP_FOREIGN_TOKEN_ADDRESS
     })	
   }
   const testBridge = new Bridge({
     assetA: 'assetAtHome',
     assetABridge: process.env.REACT_APP_HOME_MEDIATOR_ADDRESS,
     assetB: 'assetAtForeign',
     assetBBridge: process.env.REACT_APP_FOREIGN_MEDIATOR_ADDRESS
   })
   
   const core = new BurnerCore({
     signers: [new InjectedSigner(), new LocalSigner({ privateKey: process.env.REACT_APP_PK, saveKey: false })],
     gateways: [
       new InjectedGateway(),
       new TokenBridgeGateway(),
       new InfuraGateway(process.env.REACT_APP_INFURA_KEY)
     ],
     assets: [assetAtHome, assetAtForeign]
   })
   
   const exchange = new Exchange({
     pairs: [testBridge]
   })
   

   Such approach will allow to test the plugins without modifications significant modifications by providing only a correct .env file.

What do you think?

[patitonar]

Thanks for the ideas! I think they are good improvements.

Do you think it makes sense to introduce a generic gateway (TokenBridgeGateway?) similar to InfuraGateway. This gateway will supply Web3 providers for our chains: ETC, Sokol, POA. If my understanding correct, InfuraGateway make it happens by using chain id, so, we can do by the same manner.

I think it makes sense, we will be able to use different chains by importing only one gateway

What if we introduce a new asset sPOA?

We will be using it in testing so I think it's a good idea

[patitonar]

Applied the proposed improvements, please check.

Also, I found an issue that incoming transfers to an ERC677Asset were not displayed in the Recent Activity section on the main page but the balance is correctly updated. The issue happens because ERC677 abi has 2 definitions of the Transfer event, so when web3 tries to listen to it, it uses the abi for Transfer(address, address, uint, bytes) but the emitted event has the signature of Transfer(address, address, uint256) and so it does not retrieve any result.

The fix consisted of overriding the method startWatchingAddress of ERC20Asset to filter the events by the transfer topic rather than the name of the event. Just to mention, another evaluated possible solution is to use the erc20 abi to listen to the events (so there is only one Transfer definition) and only use the erc677 abi to send the transactions. Example: https://gist.github.com/patitonar/2168ba78e70f2ed472d3abddb796c1bd

Another issue I found is with the incoming native transfer of NativeAssets of burner-wallet/core. When transfers are from one account to our account it is displayed correctly in the Recent Activity section, but if the transfer is produced by the execution of a smart contract (for example a validator calling the mediator contract) it is not displayed. In all cases, the balance is correctly updated. So in our case, if the bridge has a native side when the native tokens are unlocked/minted by the mediator contract that incoming transfer won't be listed in the recent activity section.

This is because of how the code works. It looks for the latest blocks. Then looks for each transaction in the block, and check if the transaction has the value and that the to parameter is our account. Here is the code https://github.com/austintgriffith/burner-core/blob/develop/assets/src/NativeAsset.js#L75-L93

Is this something we should try to fix?

I'm not sure how can we detect those kind native transfers if it is possible. I'll try to think of an approach to fix this at least for our use case. One idea for our use case to explore could be that the bridge pair of the exchange, after performing the exchange and detecting the balance was updated on the other side, if it is a native asset, add into the history event list a new entry for this receiving transfer. But I need to think how to get all the required information, if possible, and what changes will be involved in order for the Bridge Pair to have such functionality.

[akolotov]

When transfers are from one account to our account it is displayed correctly in the Recent Activity section, but if the transfer is produced by the execution of a smart contract (for example a validator calling the mediator contract) it is not displayed.

For me, it means that the current version of BW does not reflect transfer of xDai tokens from the block reward contract. It looks to be OK for BW authors.

I'm not sure how can we detect those kind native transfers if it is possible. I'll try to think of an approach to fix this at least for our use case. One idea for our use case to explore could be that the bridge pair of the exchange, after performing the exchange and detecting the balance was updated on the other side, if it is a native asset, add into the history event list a new entry for this receiving transfer. But I need to think how to get all the required information, if possible, and what changes will be involved in order for the Bridge Pair to have such functionality.

In our cases we have two different scenarios:

1. erc20-to-native scenario. The native tokens are minted by the BlockReward contract. And there is no simple way to detect this. We could consider the combination of the event AffirmationCompleted and receive the amount of tokens by calling mintedForAccountInBlock from the reward contract pointing out in the parameter recipient from the event and the block number where the even was raised.

2. native-to-erc20 scenario. The native tokens are unlocked by the bridge/mediator contract. Again, we can consider the event AffirmationCompleted for the bridge contract. But it seems there is no similar event in case of a bridge mediator usage and we need to add something.

For both scenarios we need to watch for logs from the bridge/mediator contracts and judging from the code, the BW does not watch for the bridge logs so far at all. I have added the corresponding comment to your PR in the burner-wallet-2 repo: burner-wallet/burner-wallet-2#21 (review)

[patitonar]

erc20-to-native scenario. The native tokens are minted by the BlockReward contract. And there is no simple way to detect this. We could consider the combination of the event AffirmationCompleted and receive the amount of tokens by calling mintedForAccountInBlock from the reward contract pointing out in the parameter recipient from the event and the block number where the even was raised.

Do you think it is enough to use only the event AffirmationCompleted and get the number of tokens from the value parameter of the event?

[akolotov]

Do you think it is enough to use only the event AffirmationCompleted and get the number of tokens from the value parameter of the event?

Well, could you say how it will look like from the UX point of view? Will the user see the list of assets transfers the transaction that emits AffirmationCompleted? Or it will only see the asset transfer itself without details. The question here is because from one point of view it is not correct to say that this transaction causes the actual assets movement, from another point of view there is no such transaction at all since the native tokens are minted by the consensus mechanism.

If the transaction is not displayed in the wallet UI to provide more details about the incoming transfer, so, yes - it is enough to use only the event AffirmationCompleted and get the number of tokens from the value parameter of the event

[patitonar]

PR burner-wallet/burner-wallet-2#24 was merged and a new version was published. I updated the dependencies and now we can display information on the exchange estimate. I added a generic message mentioning the fee in 1a9ccdc

Also, added instructions on the plugin Readme to show how to install the package using yarn. 8789d3e

Some files from the commons section are not exported as part of the distribution package so they are not available. I'll look for options on how to solve this.

Regarding this issue, we were importing the abis from the common section from the monorepo. I tried some options to include them as part of the package distribution (generated dist folder) but I wasn't able to find a good way to do it since most solutions required to convert the common files to typescript to be able to compile it too. So in order to avoid modifying files from common which are used in several components, I duplicated the abis that are used into the utils folder from the plugin 5d9a353

We should publish a new preliminary version with these changes and check using it as part of https://github.com/poanetwork/tokenbridge-burner-wallet-plugin

[patitonar]

Updated the displayed message to include the fee percentage ef27817

[patitonar]

Published a new version for the npm package 0.0.2 and tested it in the other repo.

I found 2 errors.

One related to typescript types that also faced in the plugin that I thought I was able to ignore it.

Type '(value: ValueTypes) => Promise<EstimateReturn>' is not assignable to type '(value: ValueTypes) => Promise<{ estimate: string; estimateInfo: null; }>'.

I opened this PR to fix it burner-wallet/burner-wallet-2#25

The other one is related to a dependency that was not correctly specified in the package.json

TypeError: Class extends value undefined is not a constructor or null
Module.../node_modules/@poanet/tokenbridge-bw-exchange/dist/burner-wallet/gateways/TokenBridgeGateway.js

I fixed it by declaring the dependency correctly for @burner-wallet/core in cee5f93

[patitonar]

Given that the plugin does not use the common folder anymore, I simplified the Dockerfile 170767e

[patitonar]

Updated the plugin to use the latest version of @burner-wallet/exchange and published to npm version 0.0.3 of the plugin.

I updated the plugin sample repo to include the new version and no error was found this time.

[patitonar]

Addressed the comments, also updated the license and homepage attributes for the plugin 8c5faee

The README was also split and now the introduction, installation and usage sections are included in the plugin package. I think it will now be displayed in the npm page.

If changes look good and/or there are no other comments, we should publish a new npm version for the package

[akolotov]

I am OK with updating the package with the new content and even under the version 1.0.0.

[patitonar]

Published the package version 1.0.0 https://www.npmjs.com/package/@poanet/tokenbridge-bw-exchange