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 route /blocks/{blockId}/extrinsics/{extrinsicIndex} #400

Merged
merged 42 commits into from
Jan 26, 2021

Conversation

TarikGul
Copy link
Member

Feat

Added the route /blocks/{blockId}/extrinsics/{extrinsicsIndex}

Changelog

src/controllers/blocks/BlocksExtrinsicsController.ts
Added: BlocksExtrinsicsController. This controller uses the BlocksService in order to fetch the block, then retrieve the extrinsic.

src/services/blocks/BlocksService.ts
Added: fetchExtrinsicsByIndex. Fetch's the extrinsic that is requested in the params.

src/services/blocks/BlocksService.spec.ts
Added a test 'describe' suite a called fetchExrinsicsByIndex.

Additionals:
Added mock data in: src/services/test-helpers/responses/blocks/blocks789629Extrinsic.json

Notes

Inside of the test suite, I had to reuse one line of code between 2 'it' statements. This is because jest was giving me issues with using async in a describe block. It treats it as a nested asynchronous function which will throw a unhandled promise, and abort the test or give inaccurate results.

closes #382

@TarikGul TarikGul requested a review from emostov January 21, 2021 20:46
@TarikGul TarikGul changed the title Extrinsics controller feat: add route /blocks/{blockId}/extrinsics/{extrinsicsIndex} Jan 21, 2021
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

LGTM modulo some nits mostly on just on naming/ docs.

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
src/types/responses/Extrinsic.ts Outdated Show resolved Hide resolved
src/controllers/blocks/BlocksExtrinsicsController.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I think we should switch to use singular form (extrinsicIndex etc), otherwise lgtm.

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
src/controllers/blocks/BlocksExtrinsicsController.ts Outdated Show resolved Hide resolved
src/controllers/blocks/BlocksExtrinsicsController.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.spec.ts Outdated Show resolved Hide resolved
src/services/blocks/BlocksService.ts Outdated Show resolved Hide resolved
@joepetrowski
Copy link
Collaborator

I think we should switch to use singular form (extrinsicIndex etc)

I agree

TarikGul and others added 6 commits January 22, 2021 12:11
fix: typos, naming, add parseNumberOrThrow

fix: revert to parseInt
fix: cleanup block extrinsics controller

fix: omitFinalized -> true

fix: add test to check parseNumberOrThrow will throw an error if a negative is passed in.

Yarn fix
fix: update extrinsic index test to query extrinsic 2

fix: lint
@TarikGul TarikGul changed the title feat: add route /blocks/{blockId}/extrinsics/{extrinsicsIndex} feat: add route /blocks/{blockId}/extrinsics/{extrinsicIndex} Jan 23, 2021
src/controllers/blocks/BlocksExtrinsicsController.ts Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

LGTM modulo a nit

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
@TarikGul TarikGul merged commit 6507ce7 into master Jan 26, 2021
@gutenye
Copy link

gutenye commented Jan 26, 2021

@TarikGul Thank you very much!

@TarikGul
Copy link
Member Author

@gutenye Of course, thank you for bringing up the issue.

@emostov emostov deleted the extrinsics-controller branch April 14, 2021 00:24
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.

Add get extrinsic by id API
5 participants