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

List Ndau (XND) #3110

Merged
merged 1 commit into from
Oct 18, 2019
Merged

List Ndau (XND) #3110

merged 1 commit into from
Oct 18, 2019

Conversation

coriolinus
Copy link
Contributor

@coriolinus coriolinus commented Aug 20, 2019

Note: as ndau address validation is fairly complicated, this PR adds
a dependency to a library which handles this. This library can be
inspected at https://github.com/oneiro-ndev/ndauj. It is licensed
under the LGPL 3.0.

This code has been removed.

@battleofwizards
Copy link
Contributor

NACK

I guess we would rather skip the altcoin altogether (it's not even present on CMC), or have no full address validation rather than adding a new dependency.

Copy link
Contributor

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

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

NACK for adding dependency!

build.gradle Outdated Show resolved Hide resolved
@wiz
Copy link
Contributor

wiz commented Aug 22, 2019

Why would you expect Bisq to trust your 13 day old jar dependency? 🤔

@coriolinus
Copy link
Contributor Author

The jar is about 500 lines of code and easily auditable. It's 13 days old because our primary development language is go, not Java; that JAR was developed specifically to support this project.

If you prefer, it would be possible for us to inline the necessary code here. It just means adding a substantial amount more code than other altcoins have.

@coriolinus
Copy link
Contributor Author

For bringing the code into this repo, what is the preferred style: to keep all necessary code within the Ndau.java file, or to create multi-file packages as required?

@battleofwizards
Copy link
Contributor

@coriolinus inlining this dependency won't help.

@coriolinus
Copy link
Contributor Author

I guess we would rather... have no full address validation rather than adding a new dependency.

If I were to use a regex validator, it would necessarily accept addresses which the actual validator would reject: you can't check internal hash codes with a regex. That said, I do not know what the actual effects would be if the bisq validator accepted addresses which are in fact invalid. If the consequences are acceptable, it would be easy to implement.

Doing so will necessitate changing the test suite, many of the invalid cases of which are single-character replacements which invalidate the internal hash.

@battleofwizards would that be more palatable?

@coriolinus
Copy link
Contributor Author

I have updated this PR to remove the external dependency. I would appreciate a re-review of the code as it now stands.

@mrosseel
Copy link
Contributor

utACK
dependency is gone and replaced by simple regex address validation. As @coriolinus mentioned some invalid addresses can slip by this regex, but I'm not aware that this is a requirement to include an altcoin.

@ripcurlx
Copy link
Contributor

@coriolinus please resolve the conflict with Asset (because of merged new assets)

@coriolinus
Copy link
Contributor Author

@ripcurlx Done.

@ripcurlx
Copy link
Contributor

@blabno Any comments regarding our other asset requirements? Codewise it looks fine to me now.

Copy link

@blabno blabno left a comment

Choose a reason for hiding this comment

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

NACK per comments.

Squash commits into one, keeping proper commit message.

// this regex performs superficial validation, but there is a large space of addresses marked valid
// by this regex which are not in fact valid ndau addresses. For actual ndau address validation,
// use the Address class in github.com/oneiro-ndev/ndauj (java) or github.com/oneiro-ndev/ndaumath/pkg/address (go).
private static AddressValidator validator = new RegexAddressValidator("nd[anexbm][abcdefghijkmnpqrstuvwxyz23456789]{45}");
Copy link

Choose a reason for hiding this comment

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

There is no need to make this an object property. Move validator instantiation and declaration into constructor.

private static AddressValidator validator = new RegexAddressValidator("nd[anexbm][abcdefghijkmnpqrstuvwxyz23456789]{45}");

public Ndau() {
super("ndau", "XND", Ndau.validator, Network.MAINNET);
Copy link

Choose a reason for hiding this comment

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

Mainnet is the default value, so no need for that parameter.

private static AddressValidator validator = new RegexAddressValidator("nd[anexbm][abcdefghijkmnpqrstuvwxyz23456789]{45}");

public Ndau() {
super("ndau", "XND", Ndau.validator, Network.MAINNET);
Copy link

Choose a reason for hiding this comment

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

Name of asset should be uppercased I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stet: style guide rules for this asset mandate lowercase usage.

- Official project URL: https://ndau.io/
- Official block explorer URL: https://explorer.service.ndau.tech
@ripcurlx
Copy link
Contributor

@blabno Could you please re-visit this PR if it is up to be merged now? Thanks!

Copy link

@blabno blabno left a comment

Choose a reason for hiding this comment

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

ACK

@ripcurlx ripcurlx merged commit c91c40c into bisq-network:master Oct 18, 2019
@coriolinus coriolinus deleted the list-ndau branch October 18, 2019 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants