Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Wallets: validate wallet YAML file using JSON Schema #1600

Merged
merged 2 commits into from May 20, 2017

Conversation

Projects
None yet
3 participants
Contributor

harding commented May 16, 2017

As mentioned in #1592, I wanted to validate the 1,100-line wallet YAML to ensure consistency between entries. I also wanted to better document the entry format since even I sometimes wonder which option does what.

This PR validates the wallet entries against a pretty strict schema. It's not perfect---I've put in some TODOs where we could be more strict with changes to the current format---but I think it's better than nothing.

Some examples:

In #1592, I discovered Electrum declared an android platform without declaring a default mobile platform, so it wasn't being counted as a mobile wallet. If that problem hadn't already been fixed, the schema would've caught it:

user@devbco:~/bitcoin.org$ make check-validate-yaml 
The property '#/wallets/4/electrum/platform' has a property 'android' that depends on a missing property 'mobile' in schema https://bitcoin.org/quality-assurance/schemas/wallets.yaml
Makefile:299: recipe for target 'check-validate-yaml' failed
make: *** [check-validate-yaml] Error 1

In this PR, I've fixed two issues. The first was that Copay declared "Linux" as a supported OS on its Mobile platform entry. I checked #888 where we added Copay and that was never discussed, so I think it's just a copy/paste typo. The schema reported that:

user@devbco:~/bitcoin.org$ make check-validate-yaml 
The property '#/wallets/7/copay/platform/mobile/os/3' value "linux" did not match one of the following values: android, blackberry, ios, windowsphone in schema https://bitcoin.org/quality-assurance/schemas/wallets.yaml
Makefile:299: recipe for target 'check-validate-yaml' failed
make: *** [check-validate-yaml] Error 1

The other issue was that ArcBit was missing the privacynetwork field on just one of its supported platforms (Android). Again, I think this was a typo because I don't see it discussed in the PR where ArcBit was added, #1459

user@devbco:~/bitcoin.org$ make check-validate-yaml 
The property '#/wallets/26/arcbit/platform/android/privacycheck' did not contain a required property of 'privacynetwork' in schema https://bitcoin.org/quality-assurance/schemas/wallets.yaml
Makefile:299: recipe for target 'check-validate-yaml' failed
make: *** [check-validate-yaml] Error 1

In addition to the validation, this PR also links to the schema from the documentation for adding a new wallet.

quality-assurance/schemas/wallets.yaml
+ - desktop
+ properties:
+ desktop:
+ description: Wallets than run on desktop operating systems
@crwatkins

crwatkins May 16, 2017

Contributor

Typo: than->that

Contributor

crwatkins commented May 16, 2017

Concept ACK: This is the best present a wallet maintainer could wish for. I spend quite a bit of time reviewing the syntax of submissions and, more often than not, I have to have more than one offline cycle with submitters to fix the YAML. In addition, we finally have a schema to refer submitters to so that they have a better chance of getting it right in the first place. I'm excited to no longer have to check string lengths myself by hand.

Scoring change in choose-your-wallet.html: ACK

wallets.yaml schema: ACK (with slight typo comment above)

Thanks very much @harding

PR: utACK

Wallets: limit maximum description length to 418 characters
418 characters is the current longest description we have.
Contributor

harding commented May 17, 2017

@crwatkins your comment made me realize that the initial commit doesn't limit the length of the main string, the wallet description, because that's pulled from the translation YAML (e.g. en.yml) rather than the wallet YAML and it has an arbitrary variable name for each wallet and so can't easily be checked by a JSON schema without structural changes to the translation YAML file

Instead, I've pushed a commit that causes the template compile to fail if that field is too long, as well as a setting on the wallets page that lets us adjust it. Here's an example of the error produced:

Liquid die tag called. Wallet text too long  -- Error creating output
  Liquid Exception: Liquid die tag called. Wallet text too long -- Error creating output in fr/choisir-votre-porte-monnaie.html

I set the max to 418 characters, which is the longest description we have in any translation at present (BitGo in French). This is rather a bit longer than we've encouraged for English, I believe, but:

  1. I have an idea to increase the space available for detailed wallet information in the future. Hopefully it's within my meager JS skills/patience to implement.

  2. The ideal way to test these values is not with character limitations but by checking the rendered variable-width text size. For that we need something like Selenium, which is something I've done elsewhere and would like to do here eventually (but it's a big change to the testing framework).

  3. The poor French have about 20% unpronounced characters in most of their words and a profusion of short words which require lots of spaces between them. Who can blame them for long wallet descriptions? :-D

For testing this newest commit, I diffed the resultant HTML between this commit and the previous commit and verified there were only whitespace changes.

In the new commit, I also fixed the typo that @crwatkins identified (thanks!).

Contributor

crwatkins commented May 17, 2017

utACK

@wbnns wbnns self-assigned this May 19, 2017

Contributor

wbnns commented May 19, 2017

Unless others object, this will be merged on Saturday, May 20th.

Thanks guys.

@wbnns wbnns merged commit 4a25e43 into bitcoin-dot-org:master May 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment