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/consts endpoint #1210

Merged
merged 13 commits into from
Mar 1, 2023
Merged

Conversation

marshacb
Copy link
Collaborator

@marshacb marshacb commented Jan 31, 2023

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

/pallets/{palletId}/consts

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 constant instead of the entirety of each constant

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

{
	"at": {
		"hash": "0xf178fd381cbfbea8d3df1c6579cc918eb1dedeee6281cf3ee006a40e0139e469",
		"height": "14046577"
	},
	"pallet": "democracy",
	"palletIndex": "14",
	"items": [
		"EnactmentPeriod",
		"LaunchPeriod",
		"VotingPeriod",
		"VoteLockingPeriod",
		"MinimumDeposit",
		"InstantAllowed",
		"FastTrackVotingPeriod",
		"CooloffPeriod",
		"MaxVotes",
		"MaxProposals",
		"MaxDeposits",
		"MaxBlacklisted"
	]
}

/pallets/{palletId}/consts/{constItemId}

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 consts' metadata in the response

Sample response for /pallets/democracy/consts/InstantAllowed

{
	"at": {
		"hash": "0xd4b121ad968b133091b837292d7a73d77234b07be0073ad5b97a95b192d8ae3f",
		"height": "14046588"
	},
	"pallet": "democracy",
	"palletIndex": "14",
	"constantsItem": "instantAllowed"
}

@marshacb marshacb changed the title Cameron pallet constants endpoint feat: add pallets/consts endpoint Jan 31, 2023
@marshacb marshacb marked this pull request as ready for review January 31, 2023 19:02
@marshacb marshacb requested a review from a team as a code owner January 31, 2023 19:02
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.

👍

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
[palletItemIdx, palletItemMeta] = this.getEventItemMeta(
historicApi,
palletMeta as PalletMetadataV14,
palletItemIdx,
palletItemId
);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this if...else statement keeps growing and I was wondering if it would a good idea to replace this conditionals with a lookup table or an object lookup ? since the only thing that changes is the name of the function called.
I was checking some articles to find as references :

Maybe it makes it also more readable and maintainable ? Just an idea because I used that in C in the past and I think it is quite fast but I am not sure in Typescript (performance wise).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary. A lookup table would require a few abstractions, and performance isn't really going to get affected here. if...else is acceptable IMO. We know the fixed size of this if else so I don't think its going to grow much more.

That being said the only other option I would see for readability is a switch statement.

@marshacb marshacb requested a review from Imod7 February 27, 2023 14:49
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.

Looks Great!

@marshacb marshacb merged commit 97c1ca6 into master Mar 1, 2023
@marshacb marshacb deleted the cameron-pallet-constants-endpoint branch March 1, 2023 14:03
@IkerAlus IkerAlus mentioned this pull request Jul 20, 2023
10 tasks
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.

None yet

3 participants