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

Support RSK #141

Merged
merged 22 commits into from
Dec 11, 2018
Merged

Support RSK #141

merged 22 commits into from
Dec 11, 2018

Conversation

fvictorio
Copy link

This PR does some modifications that allow the bridge UI to work with RSK:

  • Don't use events to check if the transaction was processed, since RSK's public node doesn't support the getLogs RPC method.
  • Handle transaction receipt status differently (see this comment)
  • Don't show an error when events cannot be fetched. Since the stores are always updating the last events (for using them in the events section), we cannot avoid using them.

@igorbarinov
Copy link
Member

igorbarinov commented Oct 5, 2018

Deploy preview for kind-kilby-95344f processing.

Built with commit 671f054

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

@akolotov
Copy link
Collaborator

@fvictorio could you merge the changes from add-support-erc20-native to your branch as so we could test the bridge with RSK nodes?

@ghost ghost added the in progress label Oct 17, 2018
@fvictorio
Copy link
Author

Merged and resolved conflicts.

I tested that everything works in native-to-erc between Sokol and Kovan. I couldn't test it between RSK and Sokol because the bridge address I have (0x1e16aa4Df73d29C029d94CeDa3e3114EC191E25A) doesn't seem to have code anymore in the RSK testnet.

@ghost ghost removed the in progress label Oct 22, 2018
@ghost ghost assigned patitonar Nov 8, 2018
@ghost ghost added the in progress label Nov 8, 2018
@ghost ghost removed the in progress label Nov 12, 2018
@patitonar
Copy link

@akolotov e2e tests were failing because the success alert after transaction was processed was missing, so I fixed that.

The only thing is that on the success alert we used to display a message with the transaction hash (with the link to explorer) from the event that generated the transfer on the other network.

Now that we don't use events to check if the transaction was processed, we don't have a way to get that transaction hash. So for example the message only displays: Tokens received on Kovan.

If this is OK then I think all the changes are done for this PR.

If displaying the tx hash on the alert is important, then we can try using both approaches, using events when the network supports getLogs methods (in which case we can display the tx hash) and using the new approach on networks as RSK which doesn't support this method. This will add some complexity to the code but it can be done.

@akolotov
Copy link
Collaborator

@patitonar thanks for looking after this PR.

I agree that we need a parameter in the .env file to allow configuration what kind of network is used on one or another side of the bridge. My suggestion is ..._WITHOUT_EVENTS

These options will also allow to make decision if we need to display elements that depends on events: the event tab, the bridge statistic elements on the statistics tab.

@ghost ghost assigned akolotov Dec 2, 2018
@ghost ghost added the in progress label Dec 2, 2018
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.

I have added a comment as a separate note.

@ghost ghost removed the in progress label Dec 3, 2018
public/index.html Outdated Show resolved Hide resolved
@ghost ghost assigned fernandomg Dec 7, 2018
@ghost ghost added the in progress label Dec 7, 2018
@fernandomg
Copy link

@patitonar thanks for looking after this PR.

I agree that we need a parameter in the .env file to allow configuration what kind of network is used on one or another side of the bridge. My suggestion is ..._WITHOUT_EVENTS

These options will also allow to make decision if we need to display elements that depends on events: the event tab, the bridge statistic elements on the statistics tab.

Done

.env.example Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
@ghost ghost removed the in progress label Dec 10, 2018
Copy link

@patitonar patitonar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Statistics page
  1. Tokens Deposits/Withdraws sections also uses Events to get that information. So I think the Statistics tab should also not be displayed if we can't use events.
  • Status page
  1. List of authorities is generated by listening to events ValidatorAdded and ValidatorRemoved. So it shouldn't be displayed if we can't use events.
  2. On configuration section. The Authorities count is calculated from the previously mentioned list of validators. In this case I think we can use validatorCount method from BridgeValidators contract to get this information.

 - Remove link from menu
 - Redirect to `/` if trying to access directly
@ghost ghost added the in progress label Dec 10, 2018
@fernandomg
Copy link

fernandomg commented Dec 10, 2018

Tokens Deposits/Withdraws sections also uses Events to get that information. So I think the Statistics tab should also not be displayed if we can't use events.

Hidden from the menu for non-event based networks.

List of authorities is generated by listening to events ValidatorAdded and ValidatorRemoved. So it shouldn't be displayed if we can't use events.

Done

On configuration section. The Authorities count is calculated from the previously mentioned list of validators. In this case I think we can use validatorCount method from BridgeValidators contract to get this information.

Done

Also, implemented redirect for the urls that are not supported in a non-event based network. So if the user tries to access: /events it will be redirected to /; same for /statistics

@patitonar

@akolotov
Copy link
Collaborator

Do you have ideas why the deployment checks failed?

 - Remove dependency
 - Create local utils with the same evaluations than `yn`
@ghost ghost removed the in progress label Dec 11, 2018
@fernandomg
Copy link

Do you have ideas why the deployment checks failed?

Yes, yn (library to parse true/false values for .env values) is incompatible with the version of react-script that we're using in this project.

@akolotov
Copy link
Collaborator

@patitonar if you have no more comments please approve the PR and I will merge it. Thanks!

@akolotov akolotov merged commit b8a4306 into develop Dec 11, 2018
@ghost ghost removed the review label Dec 11, 2018
@akolotov akolotov deleted the support-rsk branch May 24, 2019 06:05
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.

5 participants