-
Notifications
You must be signed in to change notification settings - Fork 150
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/dispatchables endpoint #1209
Conversation
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.
Giving this a thumbs up even though I requested for review, that was by accident (until I realized we had discussed about that at
query param before).
Just some small nits, and comments but overall great job!
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.
Amazing work as always! 💯 I added some comments and suggestions that I think are quick fixes! 🙏
And I have two additional questions :
-
For the endpoint
http://127.0.0.1:8080/pallets/53/dispatchables
(connected to polkadot) why the calls are an empty array ?{ "at": { "hash": "0x726844aacf88c26d62dab9506e10fa6a943f6bd850c557c14d5179dba9618744", "height": "14430205" }, "pallet": "paraInclusion", "palletIndex": "53", "items": [] }
It is not an error since in polkadot parachains inclusion it looks like it is also empty. I was just wondering what is the use of this empty array.
-
It is not related to the
dispatchables
endpoint so if it is out of scope of this PR please let me know and I will address it at another place. For the storage endpoint :
http://127.0.0.1:8080/pallets/authorityDiscovery/storage
I get an error{ "code": 400, "message": "no queryable storage items found for palletId \"authorityDiscovery\"", "stack": "BadRequestError: no queryable storage items found for palletId \"authorityDiscovery\"\n at PalletsStorageService.findPalletMeta (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/services/AbstractPalletsService.js:94:19)\n at PalletsStorageService.fetchStorage (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/services/pallets/PalletsStorageService.js:92:50)\n at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n at async PalletsStorageController.getStorage (/Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/controllers/pallets/PalletsStorageController.js:65:57)\n at async /Users/tiny_imod/Documents/Parity/substrate-api-sidecar/build/src/controllers/AbstractController.js:226:9", "level": "error" }
while connected to polkadot. But in Substrate under Authority Discovery pallet I can find two storage items, keys and nextKeys. When I also tried to retrieve those storage items from pjs-api (docs) with
api.query.authorityDiscovery.keys
it does not work either. I was wondering why.
Thank you so much!!! 🙏
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!
I'm not exactly sure why this is empty in the polkadot codebase. Ill make a note and see if I can find a reason at some point. As for the storage question I have no idea at the moment. Since polkadot js also doesnt show those items I assume its for a reason but we probably need to check it out in more detail. |
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.
Approved witrh 2 small corrections to make (as mentioned below) :
- in the docs and
- removal of debugging statement
and 3 additional notes :
- I think it is worth adding a
break;
statement also in theerrors
andevents
loop so right after this line and this line respectively since the loop continues in the whole list of entries even if it finds the entry requested. So the same correction that was applied for thedispatchables
. - By reading Tarik s comment, it looks like there is a reason for not having the query param
at
in both dispatchables endpoints. However, I could not find a comment (as Tarik mentioned) that explains the why. It would be really useful to have an explanation if possible. 🙏 - Also, in the description of this PR in the section
/pallets/{palletId}/dispatchables/{dispatchableItemId}
it mentions query paramat
. If we do not have it maybe we could update the description by removing it ?
Summary: An endpoint that returns the dispatchables for a given pallet
/pallets/{palletId}/dispatchables
Query Params
onlyIds
: boolean to return only the Id of each dispatchable instead of the entirety of each dispatchableSample response for /pallets/democracy/dispatchables?onlyIds=true
/pallets/{palletId}/dispatchables/{dispatchableItemId}
Query Params
metadata
: boolean to choose whether to include a dispatchable's metadata in the responseSample response for /pallets/democracy/dispatchables/propose