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 starting_at option to CryptoGetAccountRecordsQuery #144

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

tinker-michaelj
Copy link
Contributor

Description:

  • Adds an option to CryptoGetAccountRecordsQuery to restrict the first payer record returned.
  • The user can restrict either:
    1. The earliest consensus time returned; or,
    2. The TransactionID of the first record returned.

Signed-off-by: tinker-michaelj <[email protected]>

/**
* If set, a restriction on the first payer record to be returned. The queried node will
* return this record (assuming one qualifies), and up to to the next 179 records, in
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the limit (180) configurable? In which case we shouldn't document 179 here, we should just say we will return all records up to some maximum amount (put it in better words, but that's the idea). Same with the 180 on line 54.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment with link to the Services property.

Comment on lines +57 to +64
/**
* The transaction id of the first record to return.
*/
TransactionID transaction_id = 3;
/**
* The earliest consensus time of the first record to return.
*/
Timestamp earliest_consensus_time = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any difference in performance depending on which of these two is used? In both cases are we going to iterate through the records looking for the first that meets the criteria?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, with current FCQ implementation, we need to iterate through all records no matter what. (If random access was supported, we could do binary search for earliest consensus time, but it isn't.)

Copy link
Member

Choose a reason for hiding this comment

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

Not sure we should add this feature until the data structure has a performant way to allow it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation already iterates through all records. 😬

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but we're adding a limit now so it will only iterate through the first K records where K is the limit and N is the total number of records. But with this feature they could pick the last transaction id so it will be O(N).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, unless it has to iterate through all to filter down by account ID. I'm guessing it's not in a map by account id.

Copy link
Contributor Author

@tinker-michaelj tinker-michaelj Jan 25, 2022

Choose a reason for hiding this comment

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

Right, these records are in an FCQueue which doesn't support random access (or even reverse traversal).

No matter how the user restricts (or doesn't restrict) the query, we must iterate through the entire FCQ.

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.

3 participants