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

xrp-ledger.toml Checker #572

Merged
merged 6 commits into from
Jul 12, 2019
Merged

xrp-ledger.toml Checker #572

merged 6 commits into from
Jul 12, 2019

Conversation

mDuo13
Copy link
Collaborator

@mDuo13 mDuo13 commented Jun 8, 2019

  • Add xrp-ledger.toml Checker tool. This reads the (defined) contents of an xrp-ledger.toml file, confirming that CORS is set up properly.
    • For accounts specified in the file, the tool checks their Domain fields against the domain being queried.
    • We can potentially add validator domain verification to this tool following the new stuff coming in v1.3.0.
  • Remove ripple.txt validator
  • Update ripple-lib to 1.2.4

For an example of a working site, enter mduo13.com to check my personal XRPL TOML file.

@MarkusTeufelberger
Copy link
Contributor

It would be nice to also have a way to put in an address on MainNet or TestNet and have the checker read the domain field, go to the correct location and verify the toml file there.

@mDuo13
Copy link
Collaborator Author

mDuo13 commented Jun 13, 2019

@MarkusTeufelberger That's a great idea. I've opened #576 to remember to do that later without expanding the scope of this task right now.

- Update ripple-lib to 1.2.4
- Redirects ripple.txt validator to the xrp-ledger.toml checker
Copy link
Contributor

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

One small fix. Overall, this looks great!

for (j=0; j<fields.length; j++) {
let fieldname = fields[j]
if (entry[fieldname] !== undefined) {
let field_def = $("<li><strong>"+fieldname+":</strong> ").appendTo(entry_def)
Copy link
Contributor

Choose a reason for hiding this comment

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

In ":</strong> ", the space is ignored/removed by jQuery. Simple fix: Move it to before </strong> like so:

": </strong>"

Copy link
Contributor

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

LGTM

@MarkusTeufelberger
Copy link
Contributor

https://ripple.com/.well-known/xrp-ledger.toml
https://xrpl.org/.well-known/xrp-ledger.toml

:-(

Would be nice to have one of these pre-filled by default instead of "example.com".

@alloynetworks
Copy link
Contributor

WIP - TOML reader (returns JSON)
https://xrptools.alloy.ee/checktoml?domain=alloy.ee
1.Will ignore toml files served without https
2. Will strip paths from the domain provided
3. Will return toml for domain with invalid certificate, but with error status.
4. Returns additional server_headers to enable comparing of Last-Modified as seen by server, and as stated in the TOML file.
5. Python source will be published after cleaning up :)

@alloynetworks
Copy link
Contributor

Address to TOML finder also active now. Again WIP
https://xrptools.alloy.ee/addresstoml?address=ADDRESS

@mDuo13
Copy link
Collaborator Author

mDuo13 commented Jul 1, 2019

@alloyxrp I was wondering why my checker wasn't displaying the information for your file; turns out the checker is right and you have a recurring syntax error in your toml file. The [[VALIDATORS], [[ACCOUNTS]], [[PRINCIPALS]], and [[SERVERS]] headings need to use double square brackets. That's TOML syntax for "array entry" whereas single square brackets indicates a fixed object. This is a case where TOML doesn't really live up to the "Obvious" in its name.

This might be something I could add to the checker to look for.

(Apologies if you get a duplicate notification: posted this in the wrong issue a moment ago.)

@alloynetworks
Copy link
Contributor

alloynetworks commented Jul 2, 2019

Not really. I would use the [[ only if I had multiple validators or accounts, like you have in your file.

I am actually using a toml python module to read the file.

I will try the various possible errors, in case the module isn't doing all the checks.

Will also check today by putting multiple validators with a single brace. I need to be more verbose in my error message.

The docs seem to be pretty clear on this. Per my understanding 😬

@mDuo13
Copy link
Collaborator Author

mDuo13 commented Jul 2, 2019

An array of tables with length 1 is not the same as a table on its own.

It's a bit more obvious if I translate it to JSON...

Single-brackets (wrong):

{
    "VALIDATORS": {
        "public_key": "n9KM73uq5BM3Fc6cxG3k5TruvbLc8Ffq17JZBmWC4uP4csL4rFST",
        "desc": "My dogfood rippled machine. Not a reliable validator."
    }
}

Double-brackets (correct):

{
    "VALIDATORS": [
        {
            "public_key": "n9KM73uq5BM3Fc6cxG3k5TruvbLc8Ffq17JZBmWC4uP4csL4rFST",
            "desc": "My dogfood rippled machine. Not a reliable validator."
        }
    ]
}

I don't think we should mix and match datatypes for this. It should always be an array, even if there's only one entry.

@alloynetworks
Copy link
Contributor

alloynetworks commented Jul 3, 2019

It's not wrong. They are both valid json. Maybe in the context of the tool, you require a list.
But fair enough. Then we must document why we insist on double braces. And may as well make the METADATA an array too.
Did you check with and without an array in my TOML reader?

@alloynetworks
Copy link
Contributor

I don't think we should mix and match datatypes for this. It should always be an array, even if there's only one entry.

@mDuo13 : Can you check https://xrptools.alloy.ee/checktoml?domain=alloy.ee now. I've changed it to double quotes (except in METADATA). I'll also add a check to warn if the rest of the sections don't have a [[....]]

@mDuo13
Copy link
Collaborator Author

mDuo13 commented Jul 10, 2019

Note for later:

If Firefox reports an XML parsing error in the toml checker, it means your web server is serving the wrong content type. Optionally add the following line to your .htaccess to make the browser use the correct content type.

AddType application/toml .toml

@alloynetworks
Copy link
Contributor

Yes, I noticed that too. I've modified my tool to check for that, and also give suggestions depending on the webserver.
Taking your observations into account about sections other than METADATA to be arrays, I'm reporting those warnings too.
https://xrptools.alloy.ee/checktoml?domain=mduo13.com

@@ -246,7 +246,7 @@ There are several prerequisites that ACME must meet for this to happen:
- ACME must control an address in the XRP Ledger. Ripple's best practices recommend using a separate issuing address and operational address. See [Issuing and Operational Addresses](issuing-and-operational-addresses.html) for details.
- ACME must enable the [DefaultRipple Flag](#defaultripple) on its issuing address for customers to send and receive its issuances.
- Alice must create an accounting relationship (trust line) from her XRP Ledger address to ACME's issuing address. She can do this from any XRP Ledger client application as long as she knows ACME's issuing address.
- ACME should publicize its issuing address on its website where customers can find it. It can also use [ripple.txt](#rippletxt) to publish the issuing address to automated systems.
- ACME should publicize its issuing address on its website where customers can find it. It can also use an [`xrp-ledger.toml` file](xrp-ledger-toml.html) to publish the issuing address to automated systems.
Copy link
Contributor

Choose a reason for hiding this comment

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

"can also" --> "should also"?

@mDuo13 mDuo13 merged commit f854352 into XRPLF:master Jul 12, 2019
@mDuo13 mDuo13 deleted the xrpltoml_validator branch December 23, 2019 21:00
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

4 participants