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

Change how sep1 presents validator information #308

Merged
merged 6 commits into from
Jun 13, 2019

Conversation

rice2000
Copy link
Contributor

@rice2000 rice2000 commented May 22, 2019

This proposal streamlines the stellar.toml specification in the wake of SEP20, essentially implementing a suggestion @MonsieurNicolas made in the discussion of that SEP.

Rather than expressing attributes of validators independently in the Account Information global fields, organizations would create a list of validators (really an array of tables) similar to the Currency Documentation section.

Each validator an organization runs would get its own entry, and relevant info would be presented per validator. This change will make it easier to add new properties to validators (if we want to), and to write tools like automated configuration generation.

This proposal also gets rid of fields that no longer match stellar core configurations (such as DESIRED_BASE_FEE and DESIRED_MAX_TX_PER_LEDGER), as well as quorum information that is more reliably discovered via SCP or via a quorum explorer like stellarbeat. There’s a tendency for that info to be out of date since organizations often update their configuration without amending their stellar.toml.

Since this is a breaking change, this proposal also adds version information as a global field to help crawlers know what fields to expect in a stellar.toml.

If adopted, this proposal would also require amending SEP20, which currently assumes nodes are declared in the Account Information global fields.

Specifically, we’d remove the following:
NODE_NAMES
OUR_VALIDATORS
ASSET_VALIDATOR
DESIRED_BASE_FEE
DESIRED_MAX_TX_PER_LEDGER
KNOWN_PEERS
HISTORY
[QUORUM_SET]

And add:
VERSION
[[VALIDATORS]]
NODE_PUBLIC_KEY
NODE_NAME
HISTORY

In addition this PR fixes #307.

This proposal streamlines the stellar.toml specification in the wake of SEP20, essentially implementing a suggestion @MonsieurNicolas made in the discussion of that SEP.

Rather than expressing attributes of validators independently in the Account Information global fields, organizations would create a list of validators (really an array of tables) similar to the Currency Documentation section.

Each validator an organization runs would get its own entry, and relevant info would be presented per validator.  This change will make it easier to add new properties to validators (if we want to), and to write tools like automated configuration generation.

This proposal also gets rid of fields that no longer match stellar core configurations (such as DESIRED_BASE_FEE and DESIRED_MAX_TX_PER_LEDGER), as well as quorum information that is more reliably discovered via SCP or via a quorum explorer like stellarbeat.  There’s a tendency for that info to be out of date since organizations often update their configuration without amending their stellar.toml.

Since this is a breaking change, this proposal also adds version information as a global field to help crawlers know what fields to expect in a stellar.toml.

If adopted, this proposal would also require amending SEP20, which currently assumes nodes are declared in the Account Information global fields.

Specifically, we’d remove the following:
NODE_NAMES
OUR_VALIDATORS
ASSET_VALIDATOR
DESIRED_BASE_FEE
DESIRED_MAX_TX_PER_LEDGER
KNOWN_PEERS
HISTORY
[QUORUM_SET]

And add:
VERSION
[[VALIDATORS]]
NODE_PUBLIC_KEY
NODE_NAME
HISTORY
@MonsieurNicolas
Copy link
Contributor

For consistency, can we drop the NODE_ prefixes for the fields under VALIDATORS:

[[VALIDATORS]]
PUBLIC_KEY="(G..."
NAME="string"
HISTORY="(optional)string"

Also, I think we should make NAME a friendly name (alphanum, no spaces allowed) and maybe complement it with an optional DESCRIPTION (if we want something else more free form)?
I think we need also PEER (to allow people to easily discover those validators).

@tomquisel
Copy link
Contributor

This is a great change!

I think @stanford-scs felt ASSET_VALIDATOR was important?

@tomerweller
Copy link
Contributor

tomerweller commented May 22, 2019

  1. This is really good.
  2. Following up on @MonsieurNicolas's proposal. I think it might be good to have both a display name and an identifier, but I wouldn't go as far as a description (we probably want to keep that in the entity level, not specific validators).

So something like:

[[VALIDATORS]]
ID="sdf.au"
DISPLAY_NAME="SDF Australia" 
PUBLIC_KEY="GCGB2S2KGYARPVIA37HYZXVRM2YZUEXA6S33ZU5BUDC6THSB62LZSTYH"
HISTORY="http://history.stellar.org/prd/core-live/core_live_001/"
  1. I'd define history as "URI" and not a "URL" (which has a web context), to accommodate things like ipfs.

@MonsieurNicolas
Copy link
Contributor

We also need to add ADDRESS to the mix and deprecate KNOWN_PEERS (unless we want to allow people to advertise other peers that are not validators, but I don't think there is a use case for this)

@tomerweller
Copy link
Contributor

tomerweller commented May 24, 2019

I think it's valuable to add a public horizon url, if there is one. But not sure how to structure this. Some options:

  1. One top level HORIZON_URL for the entity.
  2. A top level HORIZON_URLS array.
  3. HORIZON_URL per VALIDATOR.

Leaning towards 1, as the best scenario is that horizon has a load balancer that will direct you to a regional horizon/core instance.

@MonsieurNicolas what do you think?

@tomerweller
Copy link
Contributor

@rice2000, I think we need to make it clear that the TOML file describes an entity, more than it describes a domain. (we don't want entities having multiple TOML files if, for example, their validators run on multiple domains)

@rice2000
Copy link
Contributor Author

@tomerweller That makes sense to me re: clarifying entity, not domain.
@MonsieurNicolas question about PEER: by that, I think you mean the port for inbound connection. Is that correct? Is that usually called a peer in this context? Or should the field be PEER_PORT?

Tons of great suggestions here.

@MonsieurNicolas
Copy link
Contributor

@rice2000 yeah I mean replace PEER (in my first response) by ADDRESS (which deprecates the KNOWN_PEERS list)

HORIZON_URL @tomerweller yeah I don't think we should map multiple horizon clusters to stellar.toml : it also doesn't need to be an array, people that have global deployments use GSLB (so use a global address like "horizon.stellar.org" to really resolve to things like "horizon.us.stellar.org") so your option 1 seems right.

entities vs domains: I am not sure I understand the statement "validators run on different domains", in other SEPs, there is an assumption (that seems fine to me) that HOME_DOMAIN equates to the entity.
The fact that a validator may use a different domain (its ADDRESS) is irrelevant: people can use IP addresses in there if they want (thanks to SEP20, you can always reverse lookup the home domain from a validator's public key).
I think it's probably the same with the HORIZON_URL (but for this to work, we need to be able to ask Horizon for its home domain and it doesn't seem to be possible right now the '/' endpoint doesn't tell me anything about that).

Copy link
Contributor

@theaeolianmachine theaeolianmachine left a comment

Choose a reason for hiding this comment

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

All of this is really fantastic, and I definitely really appreciate all of the suggestions. I'm marking this as request changes for now as a reminder to wait for the next batch of updates, but after that I think this looks good.

- updated address definition
- changed "URL string" to "url" to be consistent with previous tables
@rice2000
Copy link
Contributor Author

rice2000 commented Jun 1, 2019

Okay: I updated to include the suggestions above.

Specifically:

  • Added HORIZON_URL to the Account Information global fields
  • Amended validator fields. Now they are ID, DISPLAY_NAME, PUBLIC_KEY, ADDRESS, and HISTORY
  • Updated the example stellar.toml accordingly.

I think that's everything, but @theaeolianmachine @MonsieurNicolas @tomerweller: please take a look when you get a chance.

@theaeolianmachine
Copy link
Contributor

Looks great to me — will wait up to 2 days (June 5th) to hear back from the other reviewers otherwise will merge this in.

@theaeolianmachine theaeolianmachine self-requested a review June 3, 2019 18:03
@sui77
Copy link
Contributor

sui77 commented Jun 4, 2019

ID, DISPLAY_NAME, PUBLIC_KEY, ADDRESS, and HISTORY

  • Naming: I'd prefer HOST instead of ADDRESS which might be confused with "Stellar Address"

@marcinx
Copy link
Contributor

marcinx commented Jun 4, 2019

+1 on Suat's ADDRESS -> HOST suggestion.

What's the purpose of the ID field? If it's a naming suggestion for TOML aliases would it be more straightforward to call it ALIAS?

@sui77
Copy link
Contributor

sui77 commented Jun 4, 2019

Agree, ALIAS sounds more meaningful.

  • Keep in mind SEP-20 Validator self verification also needs an update as soon as this one is finalized.
  • ID (or ALIAS): since example is not alphanum but contains dots , i'd clearify that in the description, too.

@rice2000
Copy link
Contributor Author

rice2000 commented Jun 5, 2019

@sui77 @marcinx I agree with changing ADDRESS to HOST
re: ID: @tomerweller I think adding that field was your suggestion. Can you clarify the purpose for me? I think calling it ALIAS makes sense, but would love your input before making that change.

@tomerweller
Copy link
Contributor

@rice2000 ALIAS sounds good

@rice2000
Copy link
Contributor Author

rice2000 commented Jun 5, 2019

Okay: I made the suggested changes.

@tomerweller
Copy link
Contributor

tomerweller commented Jun 5, 2019

Regarding ALIAS, I suggest we use an explicit regex for things to look uniform.
How about ^[a-z0-9-]{2,16}$? (lower case letters + digits + the hyphen sign, 2-16 chars)

- changed alias to a regex
- fixed sample stellar.toml to conform to spec
@rice2000
Copy link
Contributor Author

@tomerweller I used the amended regex for ALIAS: ^[a-z0-9-]+$

fixed the regex in `ALIAS` definition
Copy link
Contributor

@tomerweller tomerweller left a comment

Choose a reason for hiding this comment

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

LGTM

@marcinx
Copy link
Contributor

marcinx commented Jun 14, 2019

I think the "updated date" in the header needs to be bumped up to reflect the latest changes. It still shows: Updated: 2018-09-11

christian-rogobete added a commit to Soneso/stellar-ios-mac-sdk that referenced this pull request Aug 12, 2019
thisisalexmcgregor-bc added a commit to thisisalexmcgregor-bc/stellar-ios-mac-sdk that referenced this pull request Oct 28, 2019
…c-sdk into alex/blockchain/1.7-update

* 'master' of github.com:thisisalexmcgregor/stellar-ios-mac-sdk:
  prepare for release
  prepare for stellar protocol 12
  prepare release 1.6.8
  consider fee when creating transactions from xdr
  add optional parameter "maxOperationFee" to Transaction constructor, to be able to set the max fee per operation
  prepare for release
  - Horizon 0.19 compatibility: add "join" argument to operations and payments endpoints - add "transactionSuccessful" member to OperationResponse and to PaymentResponse - add "includeFailed" argument to operations and payments endpoints
  Horizon 0.19 compatibility: add max_fee, fee_charged fields to transaction response, depricate field fee_paid
  update toml resolver to be compatible with v2.0.0: stellar/stellar-protocol#308
  Add custom case for stellar network

# Conflicts:
#	stellarsdk/stellarsdk/responses/xdr/TransactionXDR.swift
#	stellarsdk/stellarsdk/sdk/Transaction.swift
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.

Proposal: remove [QUORUM_SET] from stellar.toml
7 participants