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

Add closed_at to ledger entry changes tables #208

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

chowbao
Copy link
Contributor

@chowbao chowbao commented Nov 2, 2023

Add closed_at to export_ledger_entry_changes tables.
The standalone "from genesis" export_* use an empty closed_at because the export_* are unused and the CheckpointChangeReader doesn't offer an easy way to get the ledger_sequence or closeMeta

@chowbao chowbao marked this pull request as ready for review November 2, 2023 18:28
@chowbao chowbao requested a review from a team as a code owner November 2, 2023 18:28
Copy link
Contributor

@sydneynotthecity sydneynotthecity left a comment

Choose a reason for hiding this comment

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

Small nit on where header is defined in some of the cmd/ files but everything is good as is

cmd/export_claimable_balances.go Outdated Show resolved Hide resolved
//if err != nil {
// logger.Fatal(fmt.Sprintf("unable to read close time for ledger %d: ", seq), err)
//}
header = changeReader.LedgerTransactionReader.GetHeader()
Copy link
Contributor

Choose a reason for hiding this comment

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

nice find :) was this method easy to find? Or would it be helpful to add to the ingest documentation for ease of use in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't necessarily obvious because of all the different reader nesting. But when you know it exists it is easy. Probably would be useful in the ingest documentation. It would be nice to get a reader/backend to xdr.* mapping like {Reader: [xdr.TxMeta, xdr.CloseMeta, xdr.LedgerHeader, etc...]} without having to look through the xdr

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea. Let's discuss with the rest of platform whether it makes sense to change the backend xdr mapping to be clearer.

@chowbao chowbao merged commit 08fd8de into master Nov 2, 2023
2 checks passed
@sydneynotthecity sydneynotthecity deleted the add-closed-at-for-ledger-entry-changes branch December 19, 2023 16:15
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.

2 participants