-
Notifications
You must be signed in to change notification settings - Fork 157
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
getLedgerEntries
: Outline building and parsing all sorts of LedgerKey
s
#1060
Conversation
Something went wrong with PR preview build please check |
2 similar comments
Something went wrong with PR preview build please check |
Something went wrong with PR preview build please check |
Something went wrong with PR preview build please check |
Preview is available here: |
Something went wrong with PR preview build please check |
Preview is available here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments inline. Looks like there's some incomplete docs here.
|
||
If you are using the [Python](https://stellar-sdk.readthedocs.io/en/latest/) `stellar_sdk` to generate these keys, you will need to install the latest version of the SDK. This can be done like so: | ||
1. **Account:** holistically defines a Stellar account, including its balance, signers, etc. (see [Accounts](https://developers.stellar.org/docs/learn/fundamentals/stellar-data-structures/accounts)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the beginning of the list, the list is describe as "forms a ledger key can take," however the list describes the ledger entries, rather than the keys. For example:
Account: holistically defines a Stellar account, including its balance, signers, etc.
Maybe:
There are 10 different forms a ledger key can take and each refers one-to-one with the following ledger entries:
|
||
##### Python | ||
<CodeExample> | ||
|
||
```python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some examples are Python and JavaScript, some are Python and TypeScript, and some are unlabelled and I assume just JavaScript. Can we make them consistent, maybe all JS instead of TS, and all also have Python?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of them are unlabeled; I'm not sure what you mean? It's all TS and/or Python. Also I don't use the Python SDK enough to make examples for them - the ones included here are copy-pasta from elsewhere iirc. Better some in Python than none imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By unlabelled, I mean that some code blocks have no context as to which SDK they're using:

And some code examples start with Python, then continue with JS/TS, where the first code sample is labeled as Python because of the tab, then the second one isn't:
Screen.Recording.2025-03-08.at.10.30.21.pm.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see what you mean by "label", I thought you meant there was no highlight. Yeah, I can see how that can be misleading. I'll add <CodeExample>
s everywhere in a new PR on Monday 👍
702461e
to
61ac3aa
Compare
Something went wrong with PR preview build please check |
3 similar comments
Something went wrong with PR preview build please check |
Something went wrong with PR preview build please check |
Something went wrong with PR preview build please check |
724fac5
to
caf2572
Compare
caf2572
to
424b47d
Compare
Preview is available here: |
1 similar comment
Preview is available here: |
I think you might want to update the description here :
"retrieval of various ledger states" doesn't seem like the terminology. Perhaps you should change it to "retrieval of various ledger entries" |
I updated it to
Since the previous sentence mentions fetching ledger entries, already.
But then you lose direct references to the XDR, no? To resolve this I added an actual direct reference and stop "pluralizing", instead, in b00970b. |
Preview is available here: |
3 similar comments
Preview is available here: |
Preview is available here: |
Preview is available here: |
@Shaptic do you need to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if you need to run yarn rpcspec:build
. But other than that, everything looks good!
Preview is available here: |
This makes the following changes:
As part of (2) I tried to morph it to be in line with the way (1) is explained. Notice that I've removed all of the base64 cruft: this is intentional as these can quickly become out of date and the guide is structured to teach someone to fish rather than giving them a base64-encoded fish.
Closes #469.
For Reviewers
I recommend viewing the entire Markdown file in this PR as-is and comparing it to the existing page rather than going line-by-line as the changes are significant and hard to parse out of a diff.
Page preview link.