-
Notifications
You must be signed in to change notification settings - Fork 151
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 finalized tag when querying blocks (Tests included) #386
Conversation
… a fork, and another to confirm a finalized block)
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.
Overall lgtm. Left minor nitpicks, questions and suggestions.
let hash; | ||
let queryFinalizedHead; | ||
|
||
if (finalized === 'false' || !this.options.finalizes) { |
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.
dumb TS question: can't we enforce finalized
to be a proper bool
with a type annotation so we don't need these === 'false'
shenanigans (and funny ternaries like on line 115 below)?
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.
As far as I know express enforces the type on a query as string | QueryString.ParsedQs
, but I think this is a great question. I believe if we were to enforce a type annotation on the query we would need to write a converter on the queryString itself, then enforce the type annotation after. @emostov Any thoughts on this.
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.
Yeah, express actually has quite a few caveats with the query params. It can be a number of different data types, so making an assumption that it will just be a string representing a boolean we can cast can be semi dangerous - it likely won't fail at runtime since it will either be a truthy or a falsey value but I think it can make things harder to debug if we make assumptions. Checkout https://evanhahn.com/gotchas-with-express-query-parsing-and-how-to-avoid-them/
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.
That was an interesting read, JS is still scary after all these years.
I'm not sure we're talking about the same thing though so re-stating it explicitly (pardon the likley wrong TS syntax):
Can't we change line 92, { query: { eventDocs, extrinsicDocs, finalized } }
, into something like this:
{ query: { eventDocs: boolean, extrinsicDocs: boolean, finalized: boolean } }
i.e. not allow "truthy stuff" at all, but only ever accept bona fide boolean
s? I realize this would require changes in the query string parsing – adding a middleware? configure a strict query string parser? – but wouldn't that be a fairly big win to us overall?
From reading the article above (TIL!) it seems like Express took a user friendly route by providing a "smart" query parser. We're an API-only app though and being strict about query parameters is a feature and not a hindrance to us. If we did make such a change and could farm out the query parsing we'd be able to clean up endpoints such as this one and also sleep better at night knowing things are locked down by default (as opposed to flexible by default).
This is not something we need to jump on right away (and definitely not in this PR) but I think it's worth surveying the space and see if there is a query parser out there that is a) well maintained and with a good reputation, and that b) plays nice with TS type annotations.
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.
Yea I agree that if we implement a middleware to almost sanitize the query types it would be a nice improvement. I think it's definitely something to benefit from when it comes to an extra layer of definitively accurate return values. Definitely something for a future PR, but also something to put some good thought into, and maybe have a chat about in the near future to design it.
In regards to the code snippet above we cant actually do:
{ query: { eventDocs: boolean, extrinsicDocs: boolean, finalized: boolean } }
Reason being, when we destructure an object like above, javascript thinks we are assigning a key eventDocs
to a value boolean
ie: a variable like const boolean = false
which is not declared anywhere.
If we were to add types on something like this it would look like:
Interface RequestHandlerExample {
query: QueryHandler
}
Interface QueryHandler {
eventDocs: string,
extrinsicDocs: string,
finalized: string
}
{ query: { eventDocs, extrinsicDocs, finalized } }: RequestHandlerExample
That also being said the types for the query are already set by the RequestHandler for the express request and response so we can't/shouldn't override them necessarily.
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.
We can try out some middleware. Not sure what is out there off the shelf, but we did build some stuff like this for Address validation and I looked into query param validation but for some reason decided not to impl.
From what I recall the best I could do was rejecting a non-boolean query param, and then in the controller you can assume its undefined | boolean
(typing overrides on the Query object would probs not be pretty because of how the @types package is defined). The reason being is that you can't change data in the express Request
object as it goes through the middleware pipeline (otherwise we could force boolean | undefined
). Its been awhile though so I could be wrong. Will make an issue
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.
Issue bool query param validation #398
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.
Ty @TarikGul for a better TS example. :)
We could definitely have a whiteboard session about this, it'd be fun actually. What I'm after is something a wee bit different than #398 (but it is starting to sound like what I'd ideally want is really hard/impossible). What I'd want is a way to make the TS compiler enforce type validation at compile time, e.g. by injecting endpoint specific middleware or by injecting code into the endpoint function directly. E.g. at compile time, look at the function signature and find too bool
s and an array of String
, inject middleware that rejects all function calls that don't respect the contract. But yeah, it doesn't seem like something that is possible today with TS.
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.
@dvdplm sounds like you want macros? Not sure what meta-programming features like this exist in TS/JS land
Co-authored-by: David <[email protected]>
Co-authored-by: David <[email protected]>
Co-authored-by: David <[email protected]>
Co-authored-by: David <[email protected]>
Co-authored-by: David <[email protected]>
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.
Nice work so far! A few changes:
- Lets make sure we don't have the finalized tag for chains that specify
this.options.finalizes === false
- I completely failed to mention but we also need to update the open api specs.
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: David <[email protected]>
Co-authored-by: David <[email protected]>
…ech/substrate-api-sidecar into finalized-headers-update
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Zeke Mostov <[email protected]>
Co-authored-by: Zeke Mostov <[email protected]>
…ech/substrate-api-sidecar into finalized-headers-update
Co-authored-by: Zeke Mostov <[email protected]>
…ech/substrate-api-sidecar into finalized-headers-update
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.
LGTM
fix: when querying for blockId and on a PoW chain omit finalized tag
…ech/substrate-api-sidecar.git; branch 'finalized-headers-update' of https://github.com/paritytech/substrate-api-sidecar into finalized-headers-update
Closes #378
Change log:
./src/services/blocks/BlocksService.ts
Added:
isFinalizedBLock
-> Private method to check whether or not a queried block is finalized.Updated:
fetchBlock
-> Added two parameters (checkFinalized: boolean
,queryFinalizedHead: boolean
). These parameters help from making unnecessary RPC calls. As a side effect as well, I added logic inside of thepromise.all
query infetchBlock
that allow us to check whether or not we should make that extra RPC call../src/controllers/blocks/BlocksController.ts
Updated:
getLatestBlock
-> Added a variablequeryFinalizedHead: boolean
, thats gets passed intofetchBlock
. Also updated the ternary logic inside of this method to harbor the new variable and also update its boolean value depending on the conditional.updated:
getBlockById
-> We added a variablecheckFinalized
that is to be passed intofetchBlock
. This checks whether or not the {blockId} is a hash or block height. When it's a block height, the value is false, and it saves us from making extra RPC calls inside ofisFinalized
../src/services/blocks/BlocksController.spec.ts
Added: Test Suite
BlockService.isFinalizedBlock
-> Checks two cases. One when the queried {blockId} is on a fork, we want to return false. Two, when the queried {blockId} is on the canon chain, and is finalized return true.Updated mock data for testing.
./src/services/test-helpers/mock/data/blocks789629Fork.json
./src/services/test-helpers/mock/mockBlock789629.ts
./src/services/test-helpers/responses/blocks/blocks789629.json
./src/types/responses/Block.ts