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 pallets/events endpoint #1204

Merged
merged 17 commits into from
Feb 13, 2023
Merged

Conversation

marshacb
Copy link
Collaborator

@marshacb marshacb commented Jan 26, 2023

Summary: An endpoint that returns the events for a given pallet

/pallets/{palletId}/events

Query Params

  • at: Which block to query, it will default to the latest finalized block. Accepts a hash or blocknumber
  • onlyIds: boolean to return only the Id of each event instead of the entirety of each event

Sample response for /pallets/democracy/events?onlyIds=true

{
	"at": {
		"hash": "0xc0403e7dcfa588573c4408a0c022c62049acf00d5cae78ef767eddf88d3dbf40",
		"height": "13974876"
	},
	"pallet": "democracy",
	"palletIndex": "14",
	"items": [
		"Proposed",
		"Tabled",
		"ExternalTabled",
		"Started",
		"Passed",
		"NotPassed",
		"Cancelled",
		"Delegated",
		"Undelegated",
		"Vetoed",
		"Blacklisted",
		"Voted",
		"Seconded",
		"ProposalCanceled"
	]
}

/pallets/{palletId}/events/{eventItemId}

Query Params

  • at: Which block to query, it will default to the latest finalized block. Accepts a hash or blocknumber
  • metadata: boolean to choose whether to include an events' metadata in the response

Sample response for /pallets/democracy/events/ExternalTabled

{
	"at": {
		"hash": "0xbb6693f2307bb78f26189640cc6a8f340a094f291e42435e4e25fb81f73bd874",
		"height": "13974886"
	},
	"pallet": "democracy",
	"palletIndex": "14",
	"eventItem": "externalTabled"
}

@marshacb marshacb changed the title Cameron pallet events endpoint feat: add pallets/events endpoint Jan 26, 2023
add unhappy path check to fetchEventItem unit tests
@marshacb marshacb marked this pull request as ready for review January 27, 2023 13:42
@marshacb marshacb requested a review from a team as a code owner January 27, 2023 13:42
@marshacb marshacb changed the title feat: add pallets/events endpoint [WIP]feat: add pallets/events endpoint Jan 30, 2023
@marshacb marshacb changed the title [WIP]feat: add pallets/events endpoint feat: add pallets/events endpoint Jan 30, 2023
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Overall, really great job just a few nits

refactored events items assignment
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Awesome, Great Job! 👍

Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

While connected to Kusama and testing the events endpoint
http://127.0.0.1:8080/pallets/12/events

I get the following result

{
  "code": 400,
  "message": "\"12\" was not recognized as a queryable pallet.",
  "stack": "BadRequestError: \"12\" was not recognized as a queryable pallet.\n    at PalletsEventsService.findPalletMeta (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/services/AbstractPalletsService.js:91:19)\n    at PalletsEventsService.fetchEvents (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/services/pallets/PalletsEventsService.js:48:50)\n    at PalletsEventsController.getEvents (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/controllers/pallets/PalletsEventsController.js:43:75)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at async /Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/controllers/AbstractController.js:226:9",
  "level": "error"
}

and this means that pallet 12 (AuthorityDiscovery pallet) has no events to query right ?

Isn't the error message

"message": "\"12\" was not recognized as a queryable pallet."

a little bit misleading though ? From this error message I understand that there is no pallet with this index instead of saying that the are no events in this pallet to query.

I believe this error message is triggered here so I was wondering if it is easy to check and add a more explanatory error message in the case of the info missing in the specific pallet. The same happens when I query the errors endpoint also. Just an idea in case it is possible.

@marshacb
Copy link
Collaborator Author

marshacb commented Feb 1, 2023

While connected to Kusama and testing the events endpoint http://127.0.0.1:8080/pallets/12/events

I get the following result

{
  "code": 400,
  "message": "\"12\" was not recognized as a queryable pallet.",
  "stack": "BadRequestError: \"12\" was not recognized as a queryable pallet.\n    at PalletsEventsService.findPalletMeta (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/services/AbstractPalletsService.js:91:19)\n    at PalletsEventsService.fetchEvents (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/services/pallets/PalletsEventsService.js:48:50)\n    at PalletsEventsController.getEvents (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/controllers/pallets/PalletsEventsController.js:43:75)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at async /Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/controllers/AbstractController.js:226:9",
  "level": "error"
}

and this means that pallet 12 (AuthorityDiscovery pallet) has no events to query right ?

Isn't the error message

"message": "\"12\" was not recognized as a queryable pallet."

a little bit misleading though ? From this error message I understand that there is no pallet with this index instead of saying that the are no events in this pallet to query.

I believe this error message is triggered here so I was wondering if it is easy to check and add a more explanatory error message in the case of the info missing in the specific pallet. The same happens when I query the errors endpoint also. Just an idea in case it is possible.

Thats correct. AuthorityDiscovery has no events currently and so we throw an error when querying for them. I've updated the error message to reflect that the items for the specific field of a pallets metadata aren't found rather than reflecting that the pallet id itself is not valid. This will also update the response for errors and other pallets endpoints.

@marshacb marshacb requested a review from Imod7 February 1, 2023 22:17
Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

When I request the events of the utility pallet while connected to Kusama :
✅ I get the response with the events when I query by pallet index
❌ I get an error message when I query by pallet name

From what I have seen this only happens for the Utility pallet.

src/services/AbstractPalletsService.ts Show resolved Hide resolved
src/services/AbstractPalletsService.ts Outdated Show resolved Hide resolved
@marshacb marshacb requested a review from Imod7 February 9, 2023 17:44
Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

Tested again and now it works like a charm! 💯 Awesome !

src/services/AbstractPalletsService.ts Show resolved Hide resolved
src/services/AbstractPalletsService.ts Outdated Show resolved Hide resolved
@marshacb marshacb merged commit 289d804 into master Feb 13, 2023
@marshacb marshacb deleted the cameron-pallet-events-endpoint branch February 13, 2023 14:45
@IkerAlus IkerAlus mentioned this pull request Jul 20, 2023
10 tasks
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.

3 participants