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

Checks (Reference changes) #296

Merged
merged 17 commits into from
Jan 26, 2018
Merged

Checks (Reference changes) #296

merged 17 commits into from
Jan 26, 2018

Conversation

mDuo13
Copy link
Collaborator

@mDuo13 mDuo13 commented Jan 24, 2018

Updates the transaction and ledger reference pages with new content for the Checks amendment due out in rippled 0.90.0. (This resolves DOC-1236.)

  • Adds CheckCreate, CheckCancel, CheckCash transaction types to the tx reference
  • Adds the tecEXPIRED code to the transaction result code list
  • Updates the OfferCreate description to describe how it'll return the new tecEXPIRED result code after the Checks amendment is enabled
  • Adds the Check object type to the ledger reference
  • Updates the Escrow concept page to mention Checks as another object that can count against the owner reserve
  • Leaves a note in the account_objects API method to update it when account_objects can't filter by checks rippled#2350 is fixed.

Also, slightly out of scope but covered too:

  • Adds more clarification of transaction finality to most transaction result classes
  • breaks up the ledger reference to use includes like the tx and api reference pages
  • Adds some example requests and responses that can be shaped into a Checks tutorial eventually. (The tutorial-checks.md page containing this stuff is intentionally not included in the dactyl config yet because it's far too rough.)

| `Amendments` | Array | STI_VECTOR256 | _(Optional)_ Array of 256-bit [amendment IDs](concept-amendments.html#about-amendments) for all currently-enabled amendments. If omitted, there are no enabled amendments. |
| `Majorities` | Array | STI_ARRAY | _(Optional)_ Array of objects describing the status of amendments that have majority support but are not yet enabled. If omitted, there are no pending amendments with majority support. |
| `Flags` | Number | UInt32 | Not used. |
| `LedgerEntryType` | String | UInt16 | The value `0x0066`, mapped to the string `Amendments`, indicates that this is object describes status of amendments to the XRP Ledger. |
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny: change "this is object" to

this object

tiny: change "describes status of amendments" to:

describes the status of amendments

| `Flags` | Number | UInt32 | Not used. |
| `LedgerEntryType` | String | UInt16 | The value `0x0066`, mapped to the string `Amendments`, indicates that this is object describes status of amendments to the XRP Ledger. |

Each member of the `Majorities` field, if it is present, is an object with a one field, `Majority`, whose contents are a nested object with the following fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny: change "object with a one field" to:

object with one field


In the [amendment process](concept-amendments.html#amendment-process), a consensus of validators adds a new amendment to the `Majorities` field using an [EnableAmendment][] pseudo-transaction with the `tfGotMajority` flag when 80% or more of validators support it. If support for a pending amendment goes below 80%, an [EnableAmendment][] pseudo-transaction with the `tfLostMajority` flag removes the amendment from the `Majorities` array. If an amendment remains in the `Majorities` field for at least 2 weeks, an [EnableAmendment][] pseudo-transaction with no flags removes it from `Majorities` and permanently adds it to the `Amendments` field.

**Note:** Technically, all transactions in a ledger are processed based on which amendments are enabled in the ledger version immediately before it. While applying transactions to a ledger version where an amendment becomes enabled, the rules don't change mid-ledger. After the ledger is closed, the next ledger uses the new rules as defined by any new amendments that applied.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this note! Useful distinction.

| `LedgerEntryType` | String | UInt16 | The value `0x0043`, mapped to the string `Check`, indicates that this object is a Check object. |
| `Account` | String | Account | The sender of the Check. Cashing the Check debits this address's balance. |
| `Destination` | String | Account | The intended recipient of the Check. Only this address can cash the Check, using a [CheckCash transaction][]. |
| `Flags` | Number | UInt32 | No flags are defined for Checks, so this value is always `0`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this descr of Flags. Do you think it would be useful to use it for Flags in amendments.md as well -- is it applicable? It is more verbose, but I think it provides good info.

| Name | JSON Type | [Internal Type][] | Description |
|-------------------|-----------|---------------|-------------|
| `LedgerEntryType` | String | UInt16 | The value `0x0064`, mapped to the string `DirectoryNode`, indicates that this object is part of a Directory. |
| `Flags` | Number | UInt32 | A bit-map of boolean flags enabled for this directory. Currently, the protocol defines no flags for DirectoryNode objects. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this Flags descr as well. Perhaps a combination of this and the one from check.md can be used to describe Flags when no flags will ever be defined for an object? Sorry, now it seems that I'm fixated on flags...LOL. I guess what I'm trying to say is that it would be good to have a comprehensive and consistent descr for when the Flag field value will always be set to 0.

| `OwnerNode` | String | UInt64 | A hint indicating which page of the owner directory links to this object, in case the directory consists of multiple pages. **Note:** The object does not contain a direct link to the owner directory containing it, since that value can be derived from the `Account`. |
| `DestinationNode` | String | UInt64 | _(Optional)_ A hint indicating which page of the destination's owner directory links to this object, in case the directory consists of multiple pages. Omitted on escrows created before enabling the [fix1523 amendment](reference-amendments.html#fix1523). |
| `PreviousTxnID` | String | Hash256 | The identifying hash of the transaction that most recently modified this object. |
| `PreviousTxnLgrSeq` | Number | UInt32 | The [index of the ledger](#ledger-index) that contains the transaction that most recently modified this object. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's necessary - does Flags need a descr in this table?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does. Adding.

@mDuo13 mDuo13 requested a review from scottschurr January 24, 2018 18:13
## RippleState
[[Source]<br>](https://github.com/ripple/rippled/blob/5d2d88209f1732a0f8d592012094e345cbe3e675/src/ripple/protocol/impl/LedgerFormats.cpp#L70 "Source")

The `RippleState` object type connects two accounts in a single currency. Conceptually, a `RippleState` object represents two _trust lines_ between the accounts, one from each side. Each account can change the settings for its side of the `RippleState` object, but the balance is a single shared value. A trust line that is entirely in its default state is considered the same as trust line that does not exist, so `rippled` deletes `RippleState` objects when their properties are entirely default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, not adding much more value than copyedits... but here's another tiny one: change "considered the same as trust line that" to:

considered the same as a trust line that

Copy link
Contributor

@jbheron jbheron left a comment

Choose a reason for hiding this comment

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

Great feature coverage! It took me a bit of time to dig into and understand, but I learned a ton (and was a nice baby-step towards writing about rippled). I made a few suggestions throughout, some which are necessary (typos for example), some which may improve the doc. Looking forward to your updates. 👍


_Requires the [Checks Amendment](reference-amendments.html#checks)._

A `Check` object describes a check, a potential pull payment, waiting to be cashed. (The potential payment has already been approved by its sender.) Example `Check` object:
Copy link
Contributor

Choose a reason for hiding this comment

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

Describing a check as a potential pull payment confuses me. Who's pulling the payment. How about comparing it to a traditional bank check, that promises payment when cashed or redeemed?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Pull payment" confused me, too, but this article helped me understand it a little more: https://www.getcontrol.co/blog/push-or-pull-payments-strategy/ (assuming that the article is correct)

Though it's interesting that the article uses checks as an example of a push payment.

| `DestinationTag` | Number | UInt32 | _(Optional)_ An arbitrary tag to further specify the destination for this Check, such as a hosted recipient at the destination address. |
| `PreviousTxnID` | String | Hash256 | The identifying hash of the transaction that most recently modified this object. |
| `PreviousTxnLgrSeq` | Number | UInt32 | The [index of the ledger](#ledger-index) that contains the transaction that most recently modified this object. |

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the object contains the fields in alphabetical order, except for the index, which occurs last. I suggest you reorder the table descriptions to parallel the example JSON.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fields in the table are presented in the following order, which I think is more useful (but please let me know if this seems wrong to you):

  • The LedgerEntryType comes first because that defines what type this object is
  • The required fields are all listed before the optional fields, except the "PreviousTxnID" and "PreviousTxnLgrSeq", which... are just out of order. Sorry, I should fix that.



### Check ID Format
[[Source]<br>](https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/Indexes.cpp#L193-L200 "Source")
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the features listing, which I expected to be the link for Checks when it was introduced, further above. Conversely, the source link in the Checks introduction contains what appears to be to me (and I don't read C++ very well) the formatting restraints, which would make sense here. Maybe you switched them or maybe I'm missing something?

The concern is a bit compounded by the fact that some of the format sections on the ledger-format page have format links to the source code, and some do not. I didn't dig further, just bringing it up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indexes.cpp is the code that defines the ID format for ledger objects. LedgerFormats.cpp is the code that defines which fields appear in which object types. I double-checked and it looks like they're linked in the correct places. C++ code can be confusing, though.

### Check ID Format
[[Source]<br>](https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/Indexes.cpp#L193-L200 "Source")

The ID of a `Check` object is the [SHA-512Half](#sha512half) of the following values put together:
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 10 uses of 'put together' on the ledger-format page, and I struggle to understand all of them, though I'm sure it is limited to one misunderstanding. Is 'put together' meant to describe concatenation? I imagine it must, and perhaps in the order of items listed in their bullets. However, I don't see examples on the page that might make it explicit. How about adding something like this editable (and inaccurate?) image:
screen shot 2018-01-24 at 12 53 09 pm

To disambiguate, I do understand that the SHA-512 algorithm acts on the elements to create the ID for that object, which also serves as the object's index (presumably for lookup in the hash map or tree).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, concatenated in the order of the bullet points is correct? Of course, it's their C++ formats, not the JSON representation, so for example the AccountID is the actual number the address is derived from, not the base-58 encoded version with a checksum that we usually write.

I've changed the wording to reflect that. I'm not adding any diagrams just yet.

@@ -557,6 +561,8 @@ See also: [Partial Payments](concept-partial-payments.html)

These codes indicate an error in the local server processing the transaction; it is possible that another server with a different configuration or load level could process the transaction successfully. They have numerical values in the range -399 to -300. The exact code for any given error is subject to change, so don't rely on it.

**Caution:** Transactions with `tel` codes are not applied to ledgers and have no effects. However, a transaction that provisionally failed may still succeed or fail with a different code after being reapplied. For more information, see [Finality of Results](#finality-of-results) and [Reliable Transaction Submission](tutorial-reliable-transaction-submission.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick:

... and have no effects.

They have no effects on what?

Is it more accurate to say that they do not take effect? Suggest:

Caution: Transactions with tel codes are not applied to ledgers and do not take effect.

Or, one may prefer:

Caution: Transactions with tel codes are not applied to ledgers and do not effect any changes to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and do not effect any changes to it.

Ooh, good use of effect as a verb, which may be too advanced for readers who aren't fluent in English.

What I'm trying to get across here is that they can't debit the XRP transaction cost, nor clean up expired things, nor anything else that you might think they would do, if they don't get applied.

I went with the following phrasing, although I'm still not completely satisfied and I'd like more suggestions for how to improve it:

cannot cause any changes to the XRP Ledger state

Copy link
Contributor

Choose a reason for hiding this comment

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

I like "cannot cause any changes to the XRP Ledger state" and think it might even be improved by terseness:

cannot cause any changes to the XRP Ledger.

@@ -0,0 +1,41 @@
## CheckCreate
[[Source]<br>](https://github.com/ripple/rippled/blob/master/src/ripple/app/tx/impl/CreateCheck.cpp "Source")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting a 404 for this link. I assume that's because it's valid on develop, but just checking that was your intention.

https://github.com/ripple/rippled/blob/develop/src/ripple/app/tx/impl/CreateCheck.cpp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As above, temporarily pointed to develop until the code is released

@@ -0,0 +1,39 @@
## CheckCash
[[Source]<br>](https://github.com/ripple/rippled/blob/master/src/ripple/app/tx/impl/CashCheck.cpp "Source")
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking that the link is as intended. I see the page on develop.

https://github.com/ripple/rippled/blob/master/src/ripple/app/tx/impl/CashCheck.cpp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It'll be in master after the release, but pointed it to develop for now so the link works before then

| `SendMax` | [Currency Amount][] | Amount | Maximum amount of source currency the Check is allowed to debit the sender, including [transfer fees](concept-transfer-fees.html) on non-XRP currencies. The Check can only credit the destination with the same currency (from the same issuer, for non-XRP currencies). For non-XRP amounts, the nested field names MUST be lower-case. |
| `Expiration` | Unsigned Integer | UInt32 | _(Optional)_ Time after which the check is no longer valid, in [seconds since the Ripple Epoch](reference-rippled.html#specifying-time). |
| `DestinationTag` | Unsigned Integer | UInt32 | _(Optional)_ Arbitrary tag that identifies the reason for the check, or a hosted recipient to pay. |
| `InvoiceID` | String | Hash256 | _(Optional)_ Arbitrary 256-bit hash representing a specific reason or identifier for this check. |
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, I'd order these table fields in parallel with either the JSON example or alphabetically (I know they come back in random order due to their hash structure, but it's nice for the reader).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reordered them to be required fields (alphabetical) then optional fields (alphabetical)

}
}
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Woah, this is really interesting in a docs-y way. I am woefully unfamiliar with using rippled, and this looks like your workspace to suss out the features so that you can write about it. Just making a side note that this is valuable to me.

}
```

## TODO: cash check (delivermin amount)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use TODO: ?

| `SignerEntries` | Array | Array | An array of SignerEntry objects representing the parties who are part of this signer list. |
| `SignerListID` | Number | UInt32 | An ID for this signer list. Currently always set to `0`. If a future [amendment](concept-amendments.html) allows multiple signer lists for an account, this may change. |
| `PreviousTxnID` | String | Hash256 | The identifying hash of the transaction that most recently modified this object. |
| `PreviousTxnLgrSeq` | Number | UInt32 | The [index of the ledger](#ledger-index) that contains the transaction that most recently modified this object. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I now understand that Flag becomes a more important and interesting field to describe when flags are actually defined for the object. 😄 For this object, I imagine that none are defined and thus it'll always be 0. Do you think this table needs a Flag row just to account for its presence in the object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, to appease @jbheron I alphabetized the fields (except for LedgerEntryType which I left first)

### Error Cases

- If the object identified by the `CheckID` does not exist or is not a Check, the transaction fails with the result `tecNO_ENTRY`.
- If the Check is not expired and the sender if the CheckCancel transaction is not the source or destination of the Check, the transaction fails with the result `tecNO_PERMISSION`.
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny: change "the sender if the CheckCancel transaction" to:

the sender of the CheckCancel transaction


Attempts to redeem a Check object in the ledger to receive up to the amount authorized by the corresponding [CheckCreate transaction][]. Only the `Destination` address of a Check can cash it with a CheckCash transaction. Cashing a check this way is similar to executing a [Payment][] initiated by the destination.

Since the funds for a check are not guaranteed, redeeming a Check can fail because the sender does not have a high enough balance or because there is not enough liquidity to deliver the funds. If this happens, the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is some text missing from the end of the para?


_Requires the [Checks Amendment](reference-amendments.html#checks)._

Create a Check object in the ledger, which is a deferred payment that can be cashed by its intended destination. The sender if this transaction is the sender of the Check.
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny: change "The sender if this transaction" to:

The sender of this transaction

Copy link
Contributor

@ryangyoung ryangyoung left a comment

Choose a reason for hiding this comment

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

I haven't finished reviewing everything but submitting my comments so far to "help" with the revisions.


The `AccountRoot` object has the following fields:

| Field | JSON Type | [Internal Type][] | Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these sorted? Should it be alphabetical?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're not really in order. (They sort of approximate the order in the source code, but not quite.)

I'll change that to be like the ledger object types: LedgerEntryType first, then required fields in alphabetical order, then optional fields in alphabetical order.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these different sorting orders explained somewhere? Seems like different tables use different sorting methods, which doesn't seem ideal for readers.

"Account": "rf1BiGeXwwQoi8Z2ueFYTEXSwuJYfV2Jpn",
"AccountTxnID": "0D5FB50FA65C9FE1538FD7E398FFFE9D1908DFA4576D8D7A020040686F93C77D",
"Balance": "148446663",
"Domain": "6D64756F31332E636F6D",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should that be 6578616d706c652e636f6d? 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe. It's probably fine because this isn't a transaction, so other people aren't going to (can't) set their domains using this example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any strong reason not to change all such examples to a standard?


| Flag Name | Hex Value | Decimal Value | Description | Corresponding [AccountSet Flag](reference-transaction-format.html#accountset-flags) |
|-----------|-----------|---------------|-------------|-------------------------------|
| lsfPasswordSpent | 0x00010000 | 65536 | Indicates that the account has used its free SetRegularKey transaction. | (None) |
Copy link
Contributor

Choose a reason for hiding this comment

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

possible nit: How are these fields sorted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In order of ascending numerical value.

Copy link
Contributor

Choose a reason for hiding this comment

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

If some tables are sorted that way and others alphabetically, we should probably explain that somewhere?


_Requires the [Checks Amendment](reference-amendments.html#checks)._

A `Check` object describes a check, a potential pull payment, waiting to be cashed. (The potential payment has already been approved by its sender.) Example `Check` object:
Copy link
Contributor

Choose a reason for hiding this comment

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

"Pull payment" confused me, too, but this article helped me understand it a little more: https://www.getcontrol.co/blog/push-or-pull-payments-strategy/ (assuming that the article is correct)

Though it's interesting that the article uses checks as an example of a push payment.


| Field | JSON Type | [Internal Type][] | Description |
|:--------------------|:-----------------|:------------------|:----------------|
| `LedgerEntryType` | String | UInt16 | The value `0x0043`, mapped to the string `Check`, indicates that this object is a Check object. |
Copy link
Contributor

Choose a reason for hiding this comment

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

One last time: maybe order these alphabetically, unless there's another, better sorting scheme already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, as with the others, I'll fix the ordering. It was originally basically in the order the fields appeared in the source. I'll change it to be LedgerEntryType first, then required fields in alphabetical, then optional fields in alphabetical order.

Copy link
Contributor

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

Docs look good. Nice work! 👍

I have some concerns about links referencing into rippled source code. The links that specify line numbers are the most fragile. But even links that reference whole files can go bad if the file moves in the source tree (although completely bad links are easier to identify with automation). Your call. I just want you to be aware.

| lsfNoFreeze | 0x00200000 | 2097152 | This address cannot freeze trust lines connected to it. Once enabled, cannot be disabled. | asfNoFreeze |
| lsfGlobalFreeze | 0x00400000 | 4194304 | All assets issued by this address are frozen. | asfGlobalFreeze |
| lsfDefaultRipple | 0x00800000 | 8388608 | Enable [rippling](concept-noripple.html) on this addresses's trust lines by default. Required for issuing addresses; discouraged for others. | asfDefaultRipple |

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that the Checks documentation comes first and the DepositAuth changes will come later. Which means the lsfDepositAuth changes will come later.



### Check ID Format
[[Source]<br>](https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/Indexes.cpp#L193-L200 "Source")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it common for the online docs to reference rippled sources? There might be some maintenance issues with that. You're referring to the file at the tip of develop. As new lines are added or removed from that file then the lines you're linking to (L193-L200) may move out from under you. I think that maintenance concern is worth thinking about seriously.

@@ -0,0 +1,39 @@
## FeeSettings
[[Source]<br>](https://github.com/ripple/rippled/blob/master/src/ripple/protocol/impl/LedgerFormats.cpp#L115-L120 "Source")
Copy link
Contributor

Choose a reason for hiding this comment

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

See earlier note about referencing the rippled sources. We're unlikely to remove any ledger formats, but we could add one in front of FeeSettings. Or, more likely, someone might decide to alphabetize them. There would be no clue that your link was incorrect unless someone did a visual inspection and knew what they were looking for.

## RippleState
[[Source]<br>](https://github.com/ripple/rippled/blob/5d2d88209f1732a0f8d592012094e345cbe3e675/src/ripple/protocol/impl/LedgerFormats.cpp#L70 "Source")

The `RippleState` object type connects two accounts in a single currency. Conceptually, a `RippleState` object represents two _trust lines_ between the accounts, one from each side. Each account can change the settings for its side of the `RippleState` object, but the balance is a single shared value. A trust line that is entirely in its default state is considered the same as a trust line that does not exist, so `rippled` deletes `RippleState` objects when their properties are entirely default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Personally, I usually think of a RippleState as representing one trust line between the accounts. Another words, I think of a trust line as being bidirectional. But I can see that it is also possible to think of it as two trust lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Historically most of our documentation and marketing stuff on trust lines has been kind of a mess, but the most common pattern was to treat trust lines as unidirectional. I decided to follow that pattern here. It's still a little messy either way you describe it.


**Note:** Prior to the introduction of the DefaultRipple flags in `rippled` version 0.27.3 (March 10, 2015), the default state for all trust lines was with both NoRipple flags disabled (rippling enabled).

Fortunately, `rippled` uses lazy evaluation to calculate the owner reserve. This means that even if an account changes the default state of all its trust lines by changing the DefaultRipple flag, that account's reserve stays the same initially. If an account modifies a trust line, `rippled` re-evaluates whether that individual trust line is in its default state and should contribute the owner reserve.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should that be "... contribute to the ..."?

@@ -70,6 +70,7 @@ There are several different kinds of objects that can appear in the ledger's sta

* [**AccountRoot** - The settings, XRP balance, and other metadata for one account.](#accountroot)
* [**Amendments** - Singleton object with status of enabled and pending amendments.](#amendments)
* [**Check** - A potential pull payment authorized by its sender](#check)
Copy link
Contributor

Choose a reason for hiding this comment

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

As someone mentioned before, the meaning of "potential pull payment" takes a while to parse. Did someone come up with a better alternative?


<!--{# Accounts used in this example:
rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo snkuWqxoqt6aeykTbkEWrTMJHrWGM (as the sender)
rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy sn2Zh1tRZyodU9qNy9tMnQr9UbBss (as the dest.)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the examples might benefit from account identifiers that have a recognizable component, at least for english speakers. For example I ran wallet_propose a few times and stumbled upon

  • "rpxQkPYe8ZU7XcYhAzWA2GyzQcpxf4SLED" (master seed "shY8V9rW1NmesV8toKCAGp3g3jXDU")
  • "rwvPiear9CeW6J3urr9jogkn8YmVhgRYE (master seed "ssaPf741VokEA4vvzrRbvexbme3P6")
  • "rnhLYqjPiNue4dPFJZ8x8rLmVuRoq6Uxrp" (master seed "shcWxfSxJYEA4j3i88E3qWuXGM8YE")

Better examples could be found with some persistence.

I'm just thinking it might make the examples a bit easier to read by distinguishing between "*SLED" and "*RYE". Just for your consideration.

@mDuo13
Copy link
Collaborator Author

mDuo13 commented Jan 26, 2018

Thanks for the review, @scottschurr. I have been thinking about the source code links and, to be honest, the current usage is all over the place. Some refer to specific commits by hash, some refer to develop, some to master, some by line number, some not... I personally find them really helpful, and our link checker catches the broken links when whole files move around. But when it comes to linking specific lines, that's harder for the reasons you mentioned.

I think I'll create a task on the docs backlog to think about and address this problem consistently, rather than fixing it in this task.

@mDuo13 mDuo13 force-pushed the checks_ref_changes branch from 5e336a0 to f0dcc2c Compare January 26, 2018 19:40
@mDuo13 mDuo13 merged commit 4967081 into XRPLF:master Jan 26, 2018
@mDuo13 mDuo13 deleted the checks_ref_changes branch June 14, 2019 20:43
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.

5 participants