Common getTokenType used on UI, monitor and Oracle

[rzadp]

• Progresses Introduce bridge-commons #8
• Created getTokenType in commons
• Moved getTokenType tests from ui to commons

[rzadp]

I'm having trouble understanding usage of getTokenType in monitor. It seems to me like there is an error.

In Oracle and UI, the first parameter of getTokenType is the bridge token contract with ERC677_BRIDGE_TOKEN_ABI, which has address public bridgeContract;

However, in monitor, foreign bridge contract is used as the first parameter of getTokenType. So the bridgeContract().call() always fails because there is no such field, and the getTokenType falls back to erc20.

So it seems to me like currently Monitor is broken and does not work with erc677 bridge token.

@akolotov @patitonar Could you please take a look?
Should we add ERC20_TOKEN_ADDRESS env variable to monitor/.env.example and check the token type in the same way as in oracle and ui?

[rzadp]

@akolotov @patitonar
On another note - In e2e-commons, we have 3 types of bridges:

1. native-to-erc
2. erc-to-native
3. erc-to-erc, with ERC20_EXTENDED_BY_ERC677=false

These 3 types of bridges are tested in oracle-e2e and ui-e2e (and pending #115 for monitor-e2e)

Should we maybe test 4th type of bridge - erc-to-erc, with ERC20_EXTENDED_BY_ERC677=true?
I imagine the error I commented above, would be caught in monitor-e2e if we tested 4 types of bridge.

[patitonar]

Good catch @rzadp! It seems the monitor is not using getTokenType correctly. As you mention it should be using ERC677_BRIDGE_TOKEN_ABI and the address of the token.

There is no need to add a new env variable since we are getting the address of the token from the foreign bridge contract in the following line:

https://github.com/poanetwork/tokenbridge/blob/5d22ad1ecb9d22968479443c95b47dd45763e8f6/monitor/utils/events.js#L38

Regarding the e2e test for this 4th bridge scenario (erc-to-erc with ERC20_EXTENDED_BY_ERC677=true) it does makes sense to me to prevent this kind of issues. If @akolotov agree, we can create an issue for this

[rzadp]

@patitonar In the line you referenced, there is erc20MethodName which is either erc677token or erc20token, regardless of the results of getTokenType.

Seems like a chicken-and-egg situation, when we cannot use getTokenType without having token address, and cannot get token address without knowing if it is erc677token or erc20token

[patitonar]

@rzadp The code is kind of confusing for this. The erc20MethodName method to use in monitor is calculated on a similar way as in UI:

https://github.com/poanetwork/tokenbridge/blob/5d22ad1ecb9d22968479443c95b47dd45763e8f6/ui/src/stores/ForeignStore.js#L184-L188

To know which method use to get the token address from the bridge contract we need to know the bridge mode, we don't need the result of getTokenType. The getTokenType method is used to get specific information from the token contract to know later how to process the events of the bridge.

[rzadp]

@patitonar Thanks for clarifying!

So, to make sure I understand:

1. Depending on bridge type, we use erc677token().call() or erc20token().call() methods to get token address - it does not determine whether the token is really erc677 or erc20.
2. Using that token address, we call getTokenType and determine whether it's actually erc677 or erc20.

It that correct?

[patitonar]

@rzadp That's correct!

[rzadp]

I've done the changes and the PR is ready for review.

[akolotov]

Should we maybe test 4th type of bridge - erc-to-erc, with ERC20_EXTENDED_BY_ERC677=true?

We already have an issue for this: #56. Let's address it separately.

[akolotov]

Please merge after @patitonar approval