Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

fix: peer validation #1621

Closed
wants to merge 13 commits into from
Closed

fix: peer validation #1621

wants to merge 13 commits into from

Conversation

dav1app
Copy link
Contributor

@dav1app dav1app commented Jan 23, 2020

Summary

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@ghost ghost added Complexity: Medium Less than 256 lines changed. Status: In Progress The issue or pull request is being worked on. labels Jan 23, 2020
@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #1621 into develop will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1621   +/-   ##
========================================
  Coverage    63.91%   63.91%           
========================================
  Files          138      138           
  Lines         3813     3808    -5     
  Branches       797      795    -2     
========================================
- Hits          2437     2434    -3     
+ Misses        1150     1148    -2     
  Partials       226      226           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49aab6e...f2856e3. Read the comment docs.

@dav1app dav1app marked this pull request as ready for review January 23, 2020 20:23
@ghost ghost added Status: Needs Review The issue or pull request needs a review by a developer of the team. and removed Status: In Progress The issue or pull request is being worked on. labels Jan 23, 2020
Copy link
Member

@alexbarnsley alexbarnsley left a comment

Choose a reason for hiding this comment

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

image

image

This error resulted in the "Validating Peer" modal from not closing on its own. In fact, I can't close the connect modal until i enter a valid peer (or reload the wallet).

To use the devnet explorer as a peer I had to use http:// with port 443:

image

src/renderer/components/Network/NetworkModal.vue Outdated Show resolved Hide resolved
src/renderer/store/modules/peer.js Outdated Show resolved Hide resolved
src/renderer/store/modules/peer.js Outdated Show resolved Hide resolved
dav1app and others added 3 commits January 29, 2020 17:53
Co-Authored-By: Alex Barnsley <8069294+alexbarnsley@users.noreply.github.com>
Co-Authored-By: Alex Barnsley <8069294+alexbarnsley@users.noreply.github.com>
Co-Authored-By: Alex Barnsley <8069294+alexbarnsley@users.noreply.github.com>
@faustbrian faustbrian changed the title Fix peer validation fix: peer validation Jan 30, 2020
@alexbarnsley
Copy link
Member

@brenopolanski could you give this a test for me please and let me know if you have any issues. Basically, test the steps in the issue which is referenced, and also try using different custom peers (e.g. https://dexplorer.ark.io). Thanks

@alexbarnsley
Copy link
Member

@davimello28 could you fix the conflicts please

@brenopolanski don't forget to review this please

@brenopolanski
Copy link
Contributor

brenopolanski commented Feb 13, 2020

@davimello28 @alexbarnsley

I did some tests using the steps in issue #1618. I realized that if you try to connect using a Connect custom peer with the following data:

Screenshot from 2020-02-13 15-40-17

It will show an error similar to this:

Screenshot from 2020-02-13 15-40-35

So close the modal, then try to connect again in ARK Devnet (on the Manage networks page). Now it will connect and this time using https:

Screenshot from 2020-02-13 15-41-28

@alexbarnsley
Copy link
Member

@brenopolanski was you on a Devnet profile when you tried the test? "Wrong nethash" should only show when connecting to a peer on a different network? E.g. a devnet peer when on a mainnet profile

@brenopolanski
Copy link
Contributor

@alexbarnsley when I did the tests I was in a mainet profile.

@alexbarnsley
Copy link
Member

@brenopolanski that error is correct then, it shouldn't be able to connect to a https://dexplorer.ark.io peer when on the mainnet profile.

Was that the only thing you came across?

@brenopolanski
Copy link
Contributor

@alexbarnsley

For now, yes. I'll investigate further and see if I look for other errors.

@alexbarnsley
Copy link
Member

@brenopolanski great, thanks. Let me know if everything works okay so I can merge this

this.$emit('connect', {
peer: this.form,
closeTrigger: this.closeTrigger
async validate () {
Copy link
Contributor

@brenopolanski brenopolanski Feb 19, 2020

Choose a reason for hiding this comment

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

The validate method is not linked to the connect button. I received the message on the browser console:

Property or method "emitConnect" is not defined on the instance but referenced during render...

Although I have updated the code, the modal does not close and does not show any alerts if the connection is successful or not.

@dav1app dav1app linked an issue Feb 20, 2020 that may be closed by this pull request
@dav1app dav1app removed a link to an issue Feb 21, 2020
@dav1app
Copy link
Contributor Author

dav1app commented Feb 26, 2020

Consider #1692.

@dav1app dav1app closed this Feb 26, 2020
@ghost ghost removed the Status: Needs Review The issue or pull request needs a review by a developer of the team. label Feb 26, 2020
@faustbrian faustbrian deleted the fix-peer-validation branch May 28, 2020 14:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Complexity: Medium Less than 256 lines changed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request made to 'api/v2/node/configuration' using HTTP instead of HTTPS
3 participants