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

fix: handle invalid address from qr code #1645

Merged
merged 5 commits into from
Feb 26, 2020
Merged

fix: handle invalid address from qr code #1645

merged 5 commits into from
Feb 26, 2020

Conversation

dated
Copy link
Contributor

@dated dated commented Feb 2, 2020

Summary

The InputDelegate component assumed that every address scanned from a qr code actually belonged to a delegate. This PR adds an additional check to make sure it does and removes the call to truncate the delegates username, since usernames are capped at a length of 20 by default.

Checklist

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

@ghost
Copy link

ghost commented Feb 2, 2020

Thanks for submitting this pull request! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@ghost ghost added Complexity: Low Less than 64 lines changed. Type: Bugfix The pull request fixes an incorrect functionality or behaviour. labels Feb 2, 2020
@brenopolanski
Copy link
Contributor

LGTM!

This PR solves the following problem, for instance:

If the wallet is using the mainnet and scanning a QR Code from a devnet delegate, the UI returns:

Screenshot from 2020-02-13 22-37-39

Using the code:

this.model = this.$store.getters['delegate/byAddress'](address).username

It will return undefined.

Screenshot from 2020-02-13 22-40-25

@brenopolanski
Copy link
Contributor

brenopolanski commented Feb 14, 2020

@dated

There is an error with classList.includes (https://github.com/ArkEcosystem/desktop-wallet/blob/develop/src/renderer/components/Input/InputDelegate.vue#L246) and you can solve here on this PR.

Screenshot from 2020-02-14 09-30-11

The Element.classList is a read-only property that returns a live DOMTokenList collection of the class attributes of the element.

Source: https://developer.mozilla.org/en-US/docs/Web/API/Element/classList

So the methods available in the classList are:

  • DOMTokenList.item(index)
  • DOMTokenList.contains(token)
  • DOMTokenList.add(token1[, token2[, ...tokenN]])
  • DOMTokenList.remove(token1[, token2[, ...tokenN]])
  • DOMTokenList.replace(oldToken, newToken)
  • DOMTokenList.supports(token)
  • DOMTokenList.toggle(token [, force])
  • DOMTokenList.entries()
  • DOMTokenList.forEach(callback [, thisArg])
  • DOMTokenList.keys()
  • DOMTokenList.values()

includes is not available in classList, so you can change to contains.

@dated
Copy link
Contributor Author

dated commented Feb 14, 2020

I'll take a look!

@dated
Copy link
Contributor Author

dated commented Feb 14, 2020

InputAddress, InputDelegate and InputSelect all have references to classList. I adjusted all three of them. The onBlur method could possibly be moved to it's own mixin, at least in InputAddress and InputDelegate it is exactly the same.

@brenopolanski
Copy link
Contributor

InputAddress, InputDelegate and InputSelect all have references to classList. I adjusted all three of them. The onBlur method could possibly be moved to it's own mixin, at least in InputAddress and InputDelegate it is exactly the same.

Works fine @dated 👍

@brenopolanski
Copy link
Contributor

brenopolanski commented Feb 14, 2020

@dated I agree with you about create a mixin for the onBlur method. Let's see what @alexbarnsley thinks about it...

@ghost
Copy link

ghost commented Feb 17, 2020

A member has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@ghost ghost added the Status: Member Approved The pull request has been approved by a member. label Feb 17, 2020
@faustbrian faustbrian added the Bounty: Tier 4 Awarded for small features, refactorings, improvements. This is valued at 20 USD. label Feb 26, 2020
@faustbrian faustbrian merged commit cf96f4b into ArkEcosystem:develop Feb 26, 2020
@ghost
Copy link

ghost commented Feb 26, 2020

Your pull request has been merged and marked as tier 4. It will earn you $20 USD.

@dated dated deleted the fix/input-delegate-qr branch February 26, 2020 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bounty: Tier 4 Awarded for small features, refactorings, improvements. This is valued at 20 USD. Complexity: Low Less than 64 lines changed. Status: Member Approved The pull request has been approved by a member. Type: Bugfix The pull request fixes an incorrect functionality or behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants