-
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
Merged
Merged
Changes from 17 commits
Commits
Show all changes
69 commits
Select commit
Hold shift + click to select a range
b42e450
Import Compact, BlockNumber Types: Create isFinalizedBlock method
TarikGul 7a2a811
Add finalized tag type
TarikGul 8a8800a
Cleanup isFinalizedBlock, add comments, add finalized tag
TarikGul 93e1f8b
Run lint --fix
TarikGul 941541e
Parallelize rpc query
TarikGul 91db51c
Update isFinalized to account for fork edgecase
TarikGul 8c35ad5
Refactor promises, and lint
TarikGul 5364626
Optimize and refactor rpc calls
TarikGul d873caf
Update blocks controller to accomodate fetchBlock params
TarikGul 39ed9c8
Refactor initial Promise.all()
TarikGul 15e48dc
Update fetchBlock in test suites to fit updated params
TarikGul 53df226
Add finalized tag with boolean tru
TarikGul 0491f27
Mock data for testing queried hashs on forks
TarikGul 1a428d4
Add tests for isFinalizedBlock: (2 tests, one is a queried hash is on…
TarikGul 1e354f7
export mock json data
TarikGul 606a578
Run lint --fix
TarikGul 53dbe74
Update grammar
TarikGul e467af0
Update src/services/blocks/BlocksService.spec.ts
TarikGul 51ae227
Update src/services/blocks/BlocksService.ts
TarikGul 643f0e6
Update src/services/blocks/BlocksService.spec.ts
TarikGul 099d85e
Update src/services/blocks/BlocksService.ts
TarikGul a914cfd
Update src/services/blocks/BlocksService.ts
TarikGul 4a19c51
Resolve comment formatting
TarikGul 18f0025
Update src/controllers/blocks/BlocksController.ts
TarikGul 45d9c97
Update params for fetchBlock to an options object
TarikGul 80d2021
BlockService resolve merge conflicts
TarikGul 056f36b
More merge conflicts resolved
TarikGul d8a2b1e
Revert changes
TarikGul 900893e
Remove BlockNumber
TarikGul 8116fd8
Revert test
TarikGul c1a4bf3
fix: Conflicts resolved, and up to date with master
TarikGul 8d756f5
update: packages, and tests to resolve for master
TarikGul b180a0b
feat: omit finalized tag when running against a PoW chain
TarikGul 270d7dc
update: update the docs
TarikGul 603b2ea
feat: add testing for omiting the finalized tag
TarikGul 6157c7b
DRY test suite
TarikGul 9215015
Update src/services/blocks/BlocksService.ts
TarikGul fae19d6
Update src/services/blocks/BlocksService.ts
TarikGul ef59d17
Update src/services/blocks/BlocksService.ts
TarikGul 57ef615
Update src/services/blocks/BlocksService.ts
TarikGul a157160
fix: lint
TarikGul 0f7c9a2
Update src/services/blocks/BlocksService.ts
TarikGul 0f018af
Update src/services/blocks/BlocksService.ts
TarikGul 1b8d82a
Update src/services/blocks/BlocksService.ts
TarikGul 308fda9
Update src/services/blocks/BlocksService.ts
TarikGul f910260
Update src/services/blocks/BlocksService.ts
TarikGul 0f31f24
Update src/services/blocks/BlocksService.ts
TarikGul 7aef28a
Update src/services/blocks/BlocksService.ts
TarikGul b1125c2
Update src/services/blocks/BlocksService.ts
TarikGul 4d915f6
Update src/controllers/blocks/BlocksController.ts
TarikGul 21105b8
fix: omitFinalizeTag => omitFinalizedTag
TarikGul 0a3f999
fix: DRY finalized
TarikGul 4e9eacb
fix: check for undefined finalizedHeadBlockNumber
TarikGul 99b536b
Merge branch 'finalized-headers-update' of https://github.com/parityt…
TarikGul 4b422e6
Update docs
TarikGul 3f4412e
fix: docs, and update the finalized description
TarikGul e34be94
Update src/controllers/blocks/BlocksController.ts
TarikGul b9aa48c
Update src/services/blocks/BlocksService.ts
TarikGul 0a34ef9
Update src/services/blocks/BlocksService.ts
TarikGul 76d5bcc
fix: DRY test code
TarikGul 3bbed72
Merge branch 'finalized-headers-update' of https://github.com/parityt…
TarikGul 37abfe4
Update src/services/blocks/BlocksService.ts
TarikGul 1fe5b31
fix: docs on interface
TarikGul 2e3d053
Merge branch 'finalized-headers-update' of https://github.com/parityt…
TarikGul bb827b3
fix: lint
TarikGul cef2f10
fix: queryFinalized should be false
TarikGul 5a8daf9
fix: queryFinalized should be false
TarikGul f71a55c
Merge branch 'finalized-headers-update' of https://github.com/parityt…
TarikGul c28b263
fix: lint
TarikGul File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
{ | ||
"header": { | ||
"hash": "0x91b171bb158e2d3848fa23a9f1c25182fb8e20313b2c1eb49219da7a70ce90c3", | ||
"parentHash": "0xfbbdd075ab413168a83dbb7108f4e75516979b0acf40e691bbdcb5706f419be6", | ||
"number": 789629, | ||
"stateRoot": "0x2ff5d2e2e90ea7af2aca2c25ec1f79643bae368cf7e0fc1cd4414ff31cf39d19", | ||
"extrinsicsRoot": "0x4c1d65bf6b57086f00d5df40aa0686ffbc581ef60878645613b1fc3303de5030", | ||
"digest": { | ||
"logs": [ | ||
{ | ||
"PreRuntime": [ | ||
1161969986, | ||
"0x036d000000f4f4d80f00000000ec9bd8e2d0368c97f3d888837f7283bbe08266869eb613159db547905026c2502a70f168b9ffcc233344005d11ebecd166769200d270a2eaa642118a00acb708a0487a440b0caf3dd5c91ab173e80ddfe5735ef8b938ea87a6105a1161612707" | ||
] | ||
}, | ||
{ | ||
"Seal": [ | ||
1161969986, | ||
"0xae78514e1de84a7d32e55b9b652f9d408ab1f7b4bfdbf6b2fad9cad94a91b86b0161cabf08f5ae1d3a1aa4993e2d96d56c94b03cee0898ccb8385a546084f88b" | ||
] | ||
} | ||
] | ||
} | ||
}, | ||
"extrinsics": [ | ||
"0x280403000bc016ed6c7301", | ||
"0x1c040a00ea313000", | ||
"0x1004140000", | ||
"0x75138486bacff9e50488125f449229ffa6767a36ab06a48b04e41c70cc7e6d82359d7d015e4a0ffba337ce7ad6e334a8199e9e574355baf90253fc216ae07fecec4b2b33526fa0e338531fec0c8178fb2335e8ca0015c7ff3eb3edf42dab16665706008585034c001a007807128889bb12ffc22c93e6190aeac259184d7181bed3f0cc9938d27315f8e61c8c4a2c0000000712040fddcb4b5b6707697e2431f7330ee99372e2a55b955bf7b93f8a853d07f10f2c0000000712f4164fa5fd5aa70d2d524d72d9e17d16a56946c3b9fa97d03d2aa2a05e25cf4e2c0000000712b4f3b0258a6c76ddaf414bedb1cbfa64eaad958a0cff4f3c57085c5df38c63042c0000000712a2c71c01fae573da0ddcf3a0f10c28e5400093c72eec182b1a141c4dfb4a8a022c000000071286bacff9e50488125f449229ffa6767a36ab06a48b04e41c70cc7e6d82359d7d2c00000007128889bb12ffc22c93e6190aeac259184d7181bed3f0cc9938d27315f8e61c8c4a2d0000000712040fddcb4b5b6707697e2431f7330ee99372e2a55b955bf7b93f8a853d07f10f2d0000000712f4164fa5fd5aa70d2d524d72d9e17d16a56946c3b9fa97d03d2aa2a05e25cf4e2d0000000712b4f3b0258a6c76ddaf414bedb1cbfa64eaad958a0cff4f3c57085c5df38c63042d0000000712a2c71c01fae573da0ddcf3a0f10c28e5400093c72eec182b1a141c4dfb4a8a022d000000071286bacff9e50488125f449229ffa6767a36ab06a48b04e41c70cc7e6d82359d7d2d00000007128889bb12ffc22c93e6190aeac259184d7181bed3f0cc9938d27315f8e61c8c4a2e0000000712040fddcb4b5b6707697e2431f7330ee99372e2a55b955bf7b93f8a853d07f10f2e0000000712f4164fa5fd5aa70d2d524d72d9e17d16a56946c3b9fa97d03d2aa2a05e25cf4e2e0000000712b4f3b0258a6c76ddaf414bedb1cbfa64eaad958a0cff4f3c57085c5df38c63042e0000000712a2c71c01fae573da0ddcf3a0f10c28e5400093c72eec182b1a141c4dfb4a8a022e000000071286bacff9e50488125f449229ffa6767a36ab06a48b04e41c70cc7e6d82359d7d2e00000007128889bb12ffc22c93e6190aeac259184d7181bed3f0cc9938d27315f8e61c8c4a2f0000000712040fddcb4b5b6707697e2431f7330ee99372e2a55b955bf7b93f8a853d07f10f2f0000000712f4164fa5fd5aa70d2d524d72d9e17d16a56946c3b9fa97d03d2aa2a05e25cf4e2f0000000712b4f3b0258a6c76ddaf414bedb1cbfa64eaad958a0cff4f3c57085c5df38c63042f0000000712a2c71c01fae573da0ddcf3a0f10c28e5400093c72eec182b1a141c4dfb4a8a022f000000071286bacff9e50488125f449229ffa6767a36ab06a48b04e41c70cc7e6d82359d7d2f00000007128889bb12ffc22c93e6190aeac259184d7181bed3f0cc9938d27315f8e61c8c4a300000000712040fddcb4b5b6707697e2431f7330ee99372e2a55b955bf7b93f8a853d07f10f300000000712f4164fa5fd5aa70d2d524d72d9e17d16a56946c3b9fa97d03d2aa2a05e25cf4e300000000712b4f3b0258a6c76ddaf414bedb1cbfa64eaad958a0cff4f3c57085c5df38c6304300000000712a2c71c01fae573da0ddcf3a0f10c28e5400093c72eec182b1a141c4dfb4a8a0230000000071286bacff9e50488125f449229ffa6767a36ab06a48b04e41c70cc7e6d82359d7d30000000", | ||
"0x510884122efdea02dda2a9b56b20809ae8108ab89c5a8692e6197d0cc8cf4903a7e81601005267278172d49fde8a23513165bcd894ba0d9b5a7ee3e2976b5202d9f2a942515021b2c175ea46139b066709dfe610ec96d019b9622f7a46ee8f2587378b8a550304001d00a569f6e4cf6bed95cafbf001e13254b28710220cd9b208c10f7154c17740099f001100305c5062779d44ea2ab0469e155b8cf3e004fce71b3b3d38263cd9fa9478f12f28f1fe9f7ba0feab9e47684d4006ed25ad6c441a1553cef62748545ea575392af5a69484f2b10ec2f1dea19394423d576f91c6b5ab2315b389f4e108bcf0aa28408e29df67564eddc299a2aa1cdacbec3c1ae061cf0835c87e5b46b920f7573a774adf51a47b72795366d52285e329229c836ea7bbfe139dbe8fa0700c4f86fc563c82ab06b794c99f14a161973be7aa6012568b1c491d45ec969ed7420bcfaa59ca5bc1915da74aba3aadd7ce7b809045d5eb5b73559259755fdcd85a40a5dc6e6af08f6bb841825b168ddf79837e70d88d75e1c5b290b74fa97cedfd668dd22c984e16482c99cfad1436111e321a86d87d0fac203bf64538f888e45d793b54133235f7d984058bb410c163fc1d7a90e5475c0917aad77deb241093a50b4f683f3ef37fce11457ebc5f00b154996471fcd30eae17efe69e5a39b12b76034dc7e4a6996ba5fce10b5419b5a9fd55998b5d7fde4922e61f93b021cb2cf6dfd5707e0f00001a93fa350e" | ||
] | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 properbool
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: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:
Reason being, when we destructure an object like above, javascript thinks we are assigning a key
eventDocs
to a valueboolean
ie: a variable likeconst boolean = false
which is not declared anywhere.If we were to add types on something like this it would look like:
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 expressRequest
object as it goes through the middleware pipeline (otherwise we could forceboolean | undefined
). Its been awhile though so I could be wrong. Will make an issueThere 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 ofString
, 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