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

Issue 2021 address validation fix #2023

Closed
wants to merge 31 commits into from
Closed

Issue 2021 address validation fix #2023

wants to merge 31 commits into from

Conversation

CaveSpectre11
Copy link
Member

Issue 2021 - Asset validation override issues in many coin extensions
  - Fix length issue in regular expression of validate() override in main asset coins.
    PIVX most likely started the 1+{25,34} that was then proliferated through the copies.
    While it likely should be 1+33, I could only confirm that 35 is too long, so the
    code is changed to be 1+{24,33} in order to reduce it by the presumed forgotten first
    character.
  - Fixup testInvalidAddresses() to better test regex rules in test asset coins.  The PIVX
    base code was testing against 3 addresses, that all had a different first character,
    therefore none of the rest of the regular expression was being tested.  Changed those
    addresses (and others for other susceptable coins) to accurately test first character
    match, length, and invalid characters in the 24 to 33 series.

Revert "List UnitedCommunityCoin (UCC)"
  - Fix length issue in regular expression of validate() override in main asset coins.
    PIVX most likely started the 1+{25,34} that was then proliferated through the copies.
    While it likely should be 1+33, I could only confirm that 35 is too long, so the
    code is changed to be 1+{24,33} in order to reduce it by the presumed forgotten first
    character.
  - Fixup testInvalidAddresses() to better test regex rules in test asset coins.  The PIVX
    base code was testing against 3 addresses, that all had a different first character,
    therefore none of the rest of the regular expression was being tested.  Changed those
    addresses (and others for other susceptable coins) to accurately test first character
    match, length, and invalid characters in the 24 to 33 series.
@CaveSpectre11
Copy link
Member Author

apologies on the extra commits; I had merged back into my master and didn't have a clean master to create a new branch from. I backed out those merges so it shouldn't have any effect; just be a little confusing. Once I save a couple other files, I'll destroy my fork and start over again. If you need me to re-apply this push with a clean fork and branch let me know and I'll take care of it.

@ManfredKarrer
Copy link
Member

Can you please make one PR for each coin. Please also consider that we are not listing ICO coins anymore. Each asset require a PR with basic info...
The fix for the existing address validation can be a normal PR. Feel free to make a compensation request for that (for the bug fix, not for the asset listing). Thanks.

@CaveSpectre11
Copy link
Member Author

@ManfredKarrer ; Just to make sure I'm understanding. The 13 address validation corrections can be done as one PR, not a separate PR per coin. I do understand this one is confusing because it shows all the commits to my master fork of the 5 coin listings, which eased testing for me; but caused issues with new branches; so I backed out the other merges; however it does look confusing.

If this is accurate (one PR for all 13), I will continue with my plan... create a new master fork and keep it clean.

Regarding compensation; is there a guide that describes what appropriate compensation amounts are ?(and where to find the wallet, although I'm sure I'll find it)

@ManfredKarrer
Copy link
Member

Yes all the bug fixed can be done in one. I was assuming you added new coins but had just a quick look. Yes a clean new PR from currnet master would be good.

Here https://github.com/bisq-network/compensation u find basic info as well as at:
https://docs.bisq.network/ and https://docs.bisq.network/dao/phase-zero.html
The BSQ address is just as kind of ID atm. You can find it in the Bisq app by cmd+d then the DAO menu opens and there u have an address from your wallet. As BSQ is not live its handled manually atm. See the docs for more background.

@CaveSpectre11
Copy link
Member Author

Ok thanks. I'll get all that figured out today. New clean PR is submitted now that I'm getting the hang of git.

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.

None yet

2 participants