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

feat: add /blocks that enforces range query param. #954

Merged
merged 29 commits into from
Jun 17, 2022
Merged

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Jun 9, 2022

Feature

Endpoint: /blocks

Query params: range (required) ex. /blocks?range=0-999

  • I implemented our own range parser because it's not perfectly clear yet if we want to provide support only for a static range such as 0-999, or for something more dynamic such as 1,2,3,10-100,123,1234

Response:

[
{<IBlock>},
{<IBlock>},
...
]

co-authored by @jsdw (Implemented the PromiseQueue)

closes: #460

@TarikGul TarikGul requested a review from a team as a code owner June 9, 2022 16:27
@TarikGul TarikGul self-assigned this Jun 11, 2022
@TarikGul
Copy link
Member Author

TarikGul commented Jun 17, 2022

Some benchmarks for querying 1000 blocks at a time against a polkadot archive node:

GET /blocks?range=0-1000 200 4242ms
GET /blocks?range=10000-11000 200 4074ms
GET /blocks?range=100000-101000 200 4578ms
GET /blocks?range=1546264-1547264 200 7568ms
GET /blocks?range=2416152-2417152 200 5778ms
GET /blocks?range=3000000-3001000 200 6084ms
GET /blocks?range=7482904-7483904 200 17009ms

@TarikGul TarikGul changed the title [WIP] feat: add /blocks that enforces range query param. feat: add /blocks that enforces range query param. Jun 17, 2022
@TarikGul TarikGul requested a review from a team June 17, 2022 03:11
const min = Number(splitRange[0]);
const max = Number(splitRange[1]);

if (!this.verifyInt(min) || !this.verifyInt(max)) {
Copy link
Member

Choose a reason for hiding this comment

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

I find this logic a little bit hard to understand.

As it will accept both 0-100 and 100-0, I kind assumed that that "reverse range" should not work.

Copy link
Member

@niklasad1 niklasad1 Jun 17, 2022

Choose a reason for hiding this comment

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

i.e,

if !(this.verifyUInt(min) {
  throw new BadRequest('Inputted `min` invalid UInt.');
) 

if !(this.verifyNonZeroUInt(min) {
  throw new BadRequest('Inputted `max` invalid NonZeroUInt.');
) 

// not sure if range(1,1) should be valid seems pointless but could work :)
if (min > max ) {
   throw new BadRequest('Invalid range: max must be bigger than min');
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea this is totally correct, will add that check.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather a have two different functions because the code becomes easier to understand as I suggested but your choice :)

Copy link
Member Author

@TarikGul TarikGul Jun 17, 2022

Choose a reason for hiding this comment

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

You mean with verifying UInt and a NonZeroUInt correct? If so, sounds totally reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, or something even better.

Copy link
Member Author

@TarikGul TarikGul Jun 17, 2022

Choose a reason for hiding this comment

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

Yea i agree, it will allow for concise behavior between the min and max as well. Will put that in now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay yea i cleaned it up to handle Uint correctly and Non zero uints correctly. I think its way cleaner and concise about what the error messages will return.

// How many tasks are currently running concurrently?
#runningTasks = 0;
// The queued tasks waiting to run
#tasks: LinkedList<() => void>;
Copy link
Member

Choose a reason for hiding this comment

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

hmm, a double ended queue should be faster than a LinkedList here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's true, the linkedlist was just easier to implement I guess, and with a double ended queue you probably want to make sure it shrinks down again when items are removed, which the linked list thing just handles :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Given this notion, do we want to implement a double ended queue, or is a linked list sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

leave it out this PR, I think the networking is the bottleneck anyway.

but maybe worth investigating :)

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
Comment on lines 220 to 222
private verifyInt(num: Number): boolean {
return Number.isInteger(num) && num > 0;
}
Copy link
Collaborator

@jsdw jsdw Jun 17, 2022

Choose a reason for hiding this comment

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

Just a random thought; I wonder how many of these sorts of functions could just be standalone utility functions and not be associated with a class in some way (once something is in a class, it has access to everything under this and so in my mind can be a little harder to reason about what they might do, whereas with standlone functions in a utility file you know exactly all of the data they will have access to).

(not suggesting anything should be done here)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea I am open to pushing this in our util folder. It's newly created with the PromiseQueue being added to sidecar so it would be nice to have these simpler verification helper functions in there.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a start to definitely clean things up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay cool, so the verifyInt has all been removed and added to its own integers util folder.

// We set a max range to 500 blocks.
const rangeOfNums = this.parseRangeOfNumbersOrThrow(range, 500);
const rangeOfNumsToHash = await Promise.all(
rangeOfNums.map(async (n) => await this.getHashForBlock(n.toString()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
rangeOfNums.map(async (n) => await this.getHashForBlock(n.toString()))
rangeOfNums.map((n) => this.getHashForBlock(n.toString()))

I guess this is identical anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

yea this is correct we dont need the async await

@jsdw
Copy link
Collaborator

jsdw commented Jun 17, 2022

Looks great!

@TarikGul TarikGul merged commit f8ab1ec into master Jun 17, 2022
@TarikGul TarikGul deleted the tarik-blocks-range branch June 17, 2022 17:07
This pull request was closed.
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.

/blocks should allow fetching of multiple blocks
3 participants