-
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
fix!: pallet storage metadataV14 #710
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.
Been a while since I look at sidecar in depth, one nit I'd fix but otherwise looks good to me!
const blockNumber = blockHeader.number.toNumber(); | ||
|
||
let adjustedMetadata; | ||
if (blockNumber < upgradeBlocks[specName.toString()]) { |
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.
In case specName.toString()
is not present in upgradeBlocks
you will be comparing a number to undefined
which, which AFAIU will be always false, hence always default to V13. Might want to make this a bit more robust?
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.
It will actually default to V14 which is what we want. I think you just got the two mixed up. For chains that aren't specified (unless they specify it themselves), we want it to default to V14
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.
This brings up a good point though, and ill test it against a outside network and see how it responds
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.
I can't say anything particularly insightful, but looks sane I think :)
@dvdplm .... Didnt push my commit with the changes to the local |
description: This gives the option to set historic blocks to use V13 metadata as oppose to V14. There are some | ||
type differences between V14 and V13 when it comes to `StorageEntryType`. Specifally the 'map' key. | ||
In turn this gives the ability to receive both older and newer storage responses while transitioning to V14. |
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.
description: This gives the option to set historic blocks to use V13 metadata as oppose to V14. There are some | |
type differences between V14 and V13 when it comes to `StorageEntryType`. Specifally the 'map' key. | |
In turn this gives the ability to receive both older and newer storage responses while transitioning to V14. | |
description: Instruct sidecar to return `StorageEntryType` in the V13 metdata format rather than V14. This is a **temporary** flag to allow existing systems to migrate. It will be deprecated and then removed in the future. |
in: query | ||
description: This gives the option to set historic blocks to use V13 metadata as oppose to V14. There are some | ||
type differences between V14 and V13 when it comes to `StorageEntryType`. Specifally the 'map' key. | ||
In turn this gives the ability to receive both older and newer storage responses while transitioning to V14. |
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.
I'd adjust along the same lines as above.
linked: false | ||
description: If the query parameter 'adjustMetadataV13' is set to true, all historic blocks that are | ||
pre v9110 will have the return type `StorageEntryTypeV13`, and all present and post v9110 blocks will | ||
have a return type of `StorageEntryTypeV14`. Please check those types to see potential responses. |
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.
Add note about the temporary nature of adjustMetadataV13
?
rel: #698 |
closes: #678
closes: #712
Since the v9110 runtime upgrade, pallets storage now has a bug when querying the endpoint. This PR takes care of that issue.
/pallets/{storageId}/*
[Added]:
query param:
adjustMetadataV13
: This allows users to receive historic responses from pallets for pre-v9110 runtimes that match sidecar's old responses forPalletMetadata
. Specifically it usesStorageEntryTypeV13
whereas V14 now returnsStorageEntryTypeV14
which has a differentasMap
.Example query:
/pallets/democracy/storage?at=700000&adjustMetadataV13=true
[Changed]
The response for
type
inside ofISanitizedStorageItemMetadata
, will now useStorageEntryTypeV14
as oppose to previous runtimes. But backwards historic compatibility will be an option, as stated above.ISanitizedStorageItemMetadata
states it's type as unknown because all previousStorageEntryType
's shared the samemap
keys, but there were many types. This should have beenStorageEntryTypeLatest
before this PR, but now it makes sense to keep it unknown.Example queries:
To demonstrate the differences between responses for historic v9110 blocks.
With
adjustMetadataV13=false
/pallets/democracy/storage?at=9625125&adjustMetadataV13=false
With
adjustMetadataV13=true
/pallets/democracy/storage?at=9625125&adjustMetadataV13=true