Fix displayed information for contracts upgraded from v1 to v2 #184

patitonar · 2019-02-08T17:34:52Z

This PR fixes the issues mentioned on omni/tokenbridge-contracts#137 (comment)

Display correctly the statistics section, including transactions from before and after contracts upgrade to v2.
Display correctly list of validators. Refactored how validator list was built. Now it process all ValidatorAdded and ValidatorRemoved in all its variations from contract version 1, version 2 and also when using Rewardable validators contract.

netlify · 2019-02-08T17:34:59Z

Deploy preview for kind-kilby-95344f processing.

Building with commit 787c17a

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

coveralls · 2019-02-08T18:13:09Z

Pull Request Test Coverage Report for Build 466

23 of 56 (41.07%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+1.9%) to 6.848%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/stores/ForeignStore.js	0	1	0.0%
src/stores/utils/contract.js	23	37	62.16%
src/stores/HomeStore.js	0	18	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/stores/HomeStore.js	1	0.0%

Totals
Change from base Build 446:	1.9%
Covered Lines:	96
Relevant Lines:	1393

💛 - Coveralls

akolotov · 2019-03-12T13:20:57Z

src/stores/HomeStore.js

+      this.processLargeArrayAsync(events, this.processEvent('UserRequestForSignature', 'AffirmationCompleted'))
+
+      // if contracts started with V1 version we should get that statistics too
+      const homeBridgeV1 = new this.homeWeb3.eth.Contract(HomeBridgeV1Abi, this.HOME_BRIDGE_ADDRESS);


Will it save time if we do this only in case if V1 contracts were deployed previously?

Yes, it will save resources if we avoid doing one extra call. The only issue I see is how to detect that V1 contracts were deployed previously

Is it possible to catch Upgraded events and find if getBridgeInterfacesVersion called for the corresponding implementation will cause error since there is no such method in the contract? My understanding is this is only the condition when the contract a) was upgraded b) upgraded from v1. What do you think? Should we make this filter even more precise?

I think that is a good strategy to detect that a contract was deployed using v1 contracts but it makes me think if it is worth applying it here. We will need to perform two requests to get this information (one for Upgraded events and one for getBridgeInterfacesVersion). Although on most cases contracts will be from v2 version and we will ended up doing this two requests only (3 requests if it is v1), I think it's better the code as it is, with just one request since it will require less time and resources to get the list of validators displayed. Does it makes sense to you?

akolotov · 2019-03-12T13:24:33Z

src/stores/utils/contract.js

+
+const getValidatorEvents = async (bridgeValidatorContract) => {
+  try {
+    return await bridgeValidatorContract.getPastEvents('allEvents', { fromBlock: 0 })


Looking the events starting from the block 0 could take lots of time. May we use deployedAtBlock() here?

Good Idea! Updated to use deployedAtBlock when we get validators events and also bridge statistics 0f29ac3

akolotov · 2019-03-26T20:45:29Z

src/stores/HomeStore.js

+      this.processLargeArrayAsync(events, this.processEvent('UserRequestForSignature', 'AffirmationCompleted'))
+
+      // if contracts started with V1 version we should get that statistics too
+      const homeBridgeV1 = new this.homeWeb3.eth.Contract(HomeBridgeV1Abi, this.HOME_BRIDGE_ADDRESS);


Is it possible to catch Upgraded events and find if getBridgeInterfacesVersion called for the corresponding implementation will cause error since there is no such method in the contract? My understanding is this is only the condition when the contract a) was upgraded b) upgraded from v1. What do you think? Should we make this filter even more precise?

akolotov · 2019-03-26T20:50:25Z

src/stores/utils/contract.js

-    return val.returnValues.validator
+export const getValidatorList = async (address, eth) => {
+  const v1Contract = new eth.Contract(BridgeValidatorsV1Abi, address);
+  const v1ValidatorsEvents = await getValidatorEvents(v1Contract)


can we get here deployedAtBlock?

akolotov · 2019-03-26T20:51:07Z

src/stores/utils/contract.js

+  const v1Contract = new eth.Contract(BridgeValidatorsV1Abi, address);
+  const v1ValidatorsEvents = await getValidatorEvents(v1Contract)
+
+  const contract = new eth.Contract(BRIDGE_VALIDATORS_ABI, address);


why do we need to use different ABIs here? I see that eventually we look for ValidatorAdded and ValidatorRemoved only

We have to use different ABIs to get the past events because the events has to match the exact definition from the ABI otherwise the event isn't returned as part of the result. We have 3 different definitions for ValidatorAdded and ValidatorRemoved.
1- On v1:

{ "anonymous": false, "inputs": [ { "indexed": false, "name": "validator", "type": "address" } ], "name": "ValidatorAdded", "type": "event" }

2- On v2 (input is indexed):

{ "anonymous": false, "inputs": [ { "indexed": true, "name": "validator", "type": "address" } ], "name": "ValidatorAdded", "type": "event" }

3- On RewardableValidators (reward field added):

{ "anonymous": false, "inputs": [ { "indexed": true, "name": "validator", "type": "address" }, { "indexed": false, "name": "reward", "type": "address" } ], "name": "ValidatorAdded", "type": "event" }

Same differences applies to ValidatorRemoved.
So for example if validators contract is on version 1, and it added a new validator. If you try to get past events using v2 abi we won't get any results

Thanks!. I see the difference between v1 and v2 contracts. I can do nothing with this - indexed was added to speed up a lookup whether the corresponding validator was added/removed.
But do you think we complicated things by adding reward field in the new versions of contracts and broke the UI functionality existed before? Do you think it makes sense to emit a separate event to notify about reward address?

I think it makes sense to emit an event where the validator and the reward address are linked as we did with ValidatorAdded on RewardableValidators.
I don't think we can benefit from emiting this information on a separate event and having two of them. So I think current implemented events are ok.

The complicated part of this was to be able to listen and parse correctly all the variations of the events when a validator is added/removed with the limitations of web3 library.
I think with the new approach applied and mentioned here #184 (comment) we found a good solution despite of having to do some low level manual parse for the events instead of letting web3 do it.

Do you recall what was the reason to have the same event name when we have different number of the event's data fields?
I am thinking (and actually we see it here) that by introducing this we added complexity and, probably, moved our design in a wrong direction. Two different events (in fact they have different signatures produced by the bytecode) for the same information cause to increase complexity in the bridge UI, in the monitor. Imagine if tomorrow we would like to synchronize the validators sets we will need to watch two different topics by the oracle. What benifit to have the same name in Solidity code in two contracts is?

That's cool that you developed a way to handle complex situation appeared due to upgrade from v1 to v2 and we definitely will use this since we have no better choice but do we want to continue increasing complexity by handling different event topics appearing in v3? I think the RewardabeValidators contract is not compatible now due to the events signature that's why we need to increase the major version?

I think the RewardabeValidators contract is not compatible now due to the events signature that's why we need to increase the major version?

I agree with this.

Do you recall what was the reason to have the same event name when we have different number of the event's data fields?

I can't remember or find the specific reason. To avoid adding complexity, do you think we should have only one event type that works with the both validators contracts needs?

Imagine if tomorrow we would like to synchronize the validators sets we will need to watch two different topics by the oracle.

Also methods to add new validator is different on BridgeValidators and RewardableValidators:

function addValidator(address _validator)

function addValidator(address _validator, address _reward)
So the complexity involves watching the two events and calling the correct methods. Maybe we should think of defining a unique API (events and methods) for both contracts

Yes, here I am on your side completely. What about addValidator and addRewardableValidator methods and leave just one event ValidatorAdded(address indexed validator)? We always can the address where rewards are going to be sent by decoding the method call parameters if it is needed in the future.

Those methods and the event sound good to me. We can also get the reward address for a validator by calling method getValidatorRewardAddress(address _validator) public view returns (address). Should we create an issue for this?

Yes, please. As soon as the issue in the contract addressed, these PR should be updated to reflect the changes in the contracts.

Created PR omni/tokenbridge-contracts#169. With changes mentioned here #184 (comment) I think there are no other changes needed for this PR

akolotov · 2019-03-26T20:54:18Z

src/stores/utils/contract.js

+  const deployedAtBlock = await getDeployedAtBlock(contract);
+  const validatorsEvents = await getValidatorEvents(contract, deployedAtBlock)
+
+  const rewardableContract = new eth.Contract(REWARDABLE_BRIDGE_VALIDATORS_ABI, address);


Does it mean that we need to use the ABI of the rewardable validator contract even if the fee manager is not in action?

Another point is: if we use the rewardable validators contract the method validatorList can be invoked to get the list of actual validators.

See this comment on why we use different ABIs #184 (comment)

Another point is: if we use the rewardable validators contract the method validatorList can be invoked to get the list of actual validators.

Thanks for the suggestion. I think there is room for improvement here. We can try to call validatorList to get the list as first intent. If it doesn't fail and return the list, there are no other required action. If it fails then we can continue to get the list of validator by listening to events from contracts using ABIs from v1 and v2. What do you think?

Could you describe a case when the list of validator by listening to events from contracts using ABIs from v1 and v2 is applicable with the new UI?

For example if we take a bridge that was deployed using v1 of contracts as POA-POA20 bridge, there are several v1 ValidatorAdded events. Then if for some reason contracts are upgraded to v2, adding or removing validators will generate v2 ValidatorAdded/ValidatorRemoved events. So we need to listen to all of that events in order to generate and display the correct list of validators. With the changes introduced mentioned here #184 (comment) we don't need anymore to make one request with v1 ABI and other with v2 ABI to get all events. Only one request will be performed and the events will be parse manually without using any of the ABIs, only the topics hashes to detect the events.

Right. But I assume that after upgrade the contracts will keep information about validators in the list so we don't need to listen events anymore at all in order to discover actual list of the bridge authorities.

My suggestion is to introduce the functionality to keep the list of validators in the BridgeValidators contract, so after the contracts upgrade from v1 to v2 we will don't need to track ValidatorAdded/ValidatorRemoved events.

PR on contracts side was created to add validatorList method to BridgeValidators contract: omni/tokenbridge-contracts#167 (comment)

Also updated the logic here 000864c

To get the list of validators now we call validatorList from validators contract. If that call fails (as in the case of xDai bridge that validators implementation doesn't have that method) or return an empty array (as in the case of ETC like chains. - tested with ETC-WETC bridge, on ETC side it returns empty array) then the logic to get validators using events is applied

Ouch! Completely forgot that the contracts for ETC was compiled with the old version of Solidity that's why we cannot return dynamic arrays from the methods there. Code features in the project trend to go our of control.

I will look at the PR in the bridge contracts repo.

# Conflicts: # src/stores/HomeStore.js # src/stores/utils/contract.js

patitonar · 2019-04-10T12:01:33Z

I made some improvements on the solution:

Now we only make one request to get all statistics of transfers including the one made from v1 version. To get all events and parse them correctly by web3 I use a combination of the bridge contract ABI and an ABI including old events names. This was implemented here 954b59d
Now only one request is made to get all validators list. I tried using a similar approach as statistics and merge all the differents ABIs (BridgeValidators, BridgeValidtorsV1 and RewardableValidators) but the problem was that web3 wasn't able to correctly parse events from BridgeValidators and BridgeValidtorsV1 because they have the same topic but v1 parameter is not indexed so returned data of event was different. So the solution was to build the contract with an empty ABI, get all events and parse them manually. In this way, with one request we can process the events and build the list even if they were generated as part of V1 contracts, V2 or by RewardableValidators. This was made here 6616bd7

In both solutions deployedAtBlock is used as fromBlock parameter to get the events

akolotov · 2019-04-10T16:01:36Z

I made some improvements on the solution:

@patitonar thanks. Sorry that I did not notice your initial responses earlier. Now I provided my comments. It could affect your recent changes. Let's discuss this in Slack if it necessary.

# Conflicts: # src/stores/ForeignStore.js # src/stores/HomeStore.js # src/stores/utils/contract.js

patitonar added 3 commits February 8, 2019 09:10

Fix statistics for contracts migrated from v1

e4c2197

Refactor how to get validator list

1b707af

Fix getValidatorEvents

e0f35e2

patitonar requested a review from akolotov February 8, 2019 17:34

igorbarinov added the in progress label Feb 8, 2019

ghost assigned patitonar Feb 8, 2019

patitonar added review and removed in progress labels Feb 8, 2019

ghost added review and removed in progress labels Feb 8, 2019

patitonar changed the title ~~Fix displayed information for contracts migrated from v1 to v2~~ Fix displayed information for contracts upgraded from v1 to v2 Feb 8, 2019

akolotov requested changes Mar 12, 2019

View reviewed changes

get events from deployedAtBlock instead of block 0

0f29ac3

ghost added the in progress label Mar 15, 2019

akolotov reviewed Mar 26, 2019

View reviewed changes

patitonar added 5 commits April 9, 2019 11:31

Merge branch 'develop' into fix-v1-to-v2-migrated-contracts

306e2eb

# Conflicts: # src/stores/HomeStore.js # src/stores/utils/contract.js

Merge fix

b69732f

Get v2 and v1 events from one request

954b59d

Refactor getValidatorList

6616bd7

Add validator events unit tests

f549b59

ghost removed the in progress label Apr 9, 2019

Move nodemon to devDependency

703c06b

ghost added the in progress label Apr 9, 2019

patitonar added 2 commits April 9, 2019 17:10

Remove unused abis

cdc29da

Remove css files

4a52bfa

ghost removed the in progress label Apr 9, 2019

Fix parseValidatorEvent

73c9ae1

ghost added the in progress label Apr 9, 2019

patitonar requested a review from akolotov April 10, 2019 12:03

patitonar mentioned this pull request Apr 11, 2019

Add validatorList on BaseBridgeValidators omni/tokenbridge-contracts#167

Merged

Use validatorList contract method

000864c

ghost removed the in progress label Apr 11, 2019

patitonar mentioned this pull request Apr 15, 2019

RewardableValidators event definitions causes additional complexity omni/tokenbridge-contracts#168

Closed

patitonar added 2 commits April 26, 2019 09:13

Merge branch 'develop' into fix-v1-to-v2-migrated-contracts

dddba46

# Conflicts: # src/stores/ForeignStore.js # src/stores/HomeStore.js # src/stores/utils/contract.js

Merge fix

787c17a

ghost added the in progress label Apr 26, 2019

akolotov approved these changes Apr 26, 2019

View reviewed changes

akolotov merged commit 75aff14 into develop Apr 26, 2019

ghost removed the in progress label Apr 26, 2019

akolotov deleted the fix-v1-to-v2-migrated-contracts branch May 24, 2019 06:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix displayed information for contracts upgraded from v1 to v2 #184

Fix displayed information for contracts upgraded from v1 to v2 #184

patitonar commented Feb 8, 2019

netlify bot commented Feb 8, 2019 •

edited

Loading

coveralls commented Feb 8, 2019 •

edited

Loading

akolotov Mar 12, 2019

patitonar Mar 15, 2019

akolotov Mar 26, 2019

patitonar Mar 27, 2019

akolotov Mar 12, 2019

patitonar Mar 15, 2019

akolotov Mar 26, 2019

akolotov Mar 26, 2019

akolotov Mar 26, 2019

patitonar Mar 27, 2019

akolotov Apr 10, 2019

patitonar Apr 10, 2019

akolotov Apr 10, 2019

patitonar Apr 11, 2019

akolotov Apr 12, 2019

patitonar Apr 12, 2019

akolotov Apr 12, 2019

patitonar Apr 15, 2019

akolotov Mar 26, 2019

patitonar Mar 27, 2019

akolotov Apr 10, 2019

patitonar Apr 10, 2019

akolotov Apr 10, 2019

patitonar Apr 11, 2019

akolotov Apr 12, 2019

akolotov Apr 12, 2019

patitonar commented Apr 10, 2019 •

edited

Loading

akolotov commented Apr 10, 2019

Fix displayed information for contracts upgraded from v1 to v2 #184

Fix displayed information for contracts upgraded from v1 to v2 #184

Conversation

patitonar commented Feb 8, 2019

netlify bot commented Feb 8, 2019 • edited Loading

coveralls commented Feb 8, 2019 • edited Loading

Pull Request Test Coverage Report for Build 466

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patitonar commented Apr 10, 2019 • edited Loading

akolotov commented Apr 10, 2019

netlify bot commented Feb 8, 2019 •

edited

Loading

coveralls commented Feb 8, 2019 •

edited

Loading

patitonar commented Apr 10, 2019 •

edited

Loading