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

working fetcher CLI #60

Merged
merged 5 commits into from
May 17, 2024
Merged

working fetcher CLI #60

merged 5 commits into from
May 17, 2024

Conversation

konnov
Copy link
Contributor

@konnov konnov commented May 17, 2024

This PR adds relatively small changes on top of #55. The fetcher is fetching the transactions and saving them in the storage. For example:

$ solarkraft fetch --rpc https://horizon-testnet.stellar.org --id CC22QGTOUMERDNIYN7TPNX3V6EMPHQXVSRR3XY56EADF7YTFISD2ROND --height 1638368
Target contract: CC22QGTOUMERDNIYN7TPNX3V6EMPHQXVSRR3XY56EADF7YTFISD2ROND...
Last cached height: 1638598
Fetching fresh transactions from: https://horizon-testnet.stellar.org...
Fetching the ledger for 1638368
Fetching transactions indefinitely. Close with Ctrl-C.
+ save: 1638369
+ save: 1638371
+ save: 1638370
+ save: 1638372
+ save: 1638373
+ save: 1638374
+ save: 1638375
+ save: 1638376
+ save: 1638378
+ save: 1638377
+ save: 1638379
+ save: 1638380
+ save: 1638381
+ save: 1638382
+ save: 1638383
= at: 1638412
= at: 1638456

I will add an integration test for the above later today.

@konnov konnov self-assigned this May 17, 2024
@konnov konnov added the enhancement New feature or request label May 17, 2024
@konnov konnov added this to the M1: Transaction extractor milestone May 17, 2024
Copy link
Collaborator

@thpani thpani left a comment

Choose a reason for hiding this comment

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

I have a few comments, but nothing blocking.

Thanks for pushing this through 🙏

* For every contract id, store the ledger height,
* up to which the transactions were fetched.
*/
heights: OrderedMap<string, number>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use bigint as the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also thought about it, but Stellar SDK is using number so we are compatible with them: https://stellar.github.io/js-stellar-sdk/OperationCallBuilder.html#forLedger

solarkraft/src/fetcher/storage.ts Outdated Show resolved Hide resolved
solarkraft/src/fetcher/storage.ts Outdated Show resolved Hide resolved
Comment on lines 185 to 186
function getFetcherStateFilename(root: string) {
mkdirSync(root, { recursive: true })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really obvious that this has side-effects from the function name.

Can we just call mkdirSync in saveFetcherState instead of here?

Comment on lines 32 to 35
if (args.height < 0) {
// how to fetch the latest height?
console.log('not implemented yet, starting with 0')
lastHeight = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this also start from the cached height?

Comment on lines 84 to 87
// load and save the state, other fetchers may work concurrently
fetcherState = loadFetcherState(args.home)
fetcherState = { ...fetcherState, heights: fetcherState.heights.set(contractId, lastHeight) }
saveFetcherState(args.home, fetcherState)
Copy link
Collaborator

@thpani thpani May 17, 2024

Choose a reason for hiding this comment

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

Don't you get a filesystem race here with concurrent fetchers? 😄

And actually, will the streaming callbacks be processed sequentially?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, there is a possibility into missing filesystem updates for concurrently running fetchers. Also, for the callbacks in theory.

Comment on lines 78 to 80
if (nEvents % HEIGHT_FETCHING_PERIOD === 0) {
// Fetch the height of the current message.
// Note that messages may come slightly out of order, so the heights are not precise.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not entirely sure what this block does.

afaict, every 100 messages, it will actually fetch the tx blockheight, and updated the persisted state? May be worth a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I will add a comment

@konnov konnov merged commit c900229 into main May 17, 2024
3 checks passed
@konnov konnov deleted the igor/fetcher-loop-really branch May 17, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants