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

update MMR API #5479

Merged
merged 8 commits into from
Mar 23, 2023
Merged

update MMR API #5479

merged 8 commits into from
Mar 23, 2023

Conversation

Lederstrumpf
Copy link
Contributor

fixes #5478

@Lederstrumpf
Copy link
Contributor Author

I've tested the API methods here: https://github.com/Lederstrumpf/polkadot-js-api-ts-template/blob/test-mmr-rpc/src/index.ts

I'm also fine to remove the mmr_root method here to reduce the API's surface since it can equivalently be performed with historic_api.query.mmr.rootHash(); I added for completeness since it's provided by RPC nodes.

jacogr
jacogr previously requested changes Mar 20, 2023
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Really appreciate all the effort here: thank you.

Could you please just merge in master, GH shows some conflicts.

the signature's changed in v2 for all runtime calls that they share.
@Lederstrumpf
Copy link
Contributor Author

Really appreciate all the effort here: thank you.

gladly!

Could you please just merge in master, GH shows some conflicts.

Done. As a followup to the merge, I've fully decoupled the MMR runtime methods wrt versioning in ff99764 (previously MMR_V1_V2 on fresh master). Since all of the methods' signatures changed from v1 to v2, there's no intersection between those versions. So this struck me as the most correct approach under the circumstances, but please let me know if I'm wrong here or you prefer it differently.

@jacogr
Copy link
Member

jacogr commented Mar 23, 2023

It is quite possible I mucked up something when adding the MMR V1/V2 split. Was also not quite comfortable with it anyway since it seemed that methods were added and then the version was only bumped afterwards - this could quite possibly just be the update schedule followed when these were adjusted.

Anyway, I'm much happier when it comes "from the source of knowledge" on these since the intent is much clearer.

jacogr
jacogr previously requested changes Mar 23, 2023
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Some small linting fixes suggested (as per CI failures) - there is however still an ordering issue, couldn't do that is the GH interface without mucking up.

packages/types/src/interfaces/mmr/rpc.ts Outdated Show resolved Hide resolved
packages/types/src/interfaces/mmr/rpc.ts Outdated Show resolved Hide resolved
packages/types/src/interfaces/mmr/rpc.ts Outdated Show resolved Hide resolved
packages/types/src/interfaces/mmr/runtime.ts Outdated Show resolved Hide resolved
packages/types/src/interfaces/mmr/runtime.ts Outdated Show resolved Hide resolved
@Lederstrumpf
Copy link
Contributor Author

It is quite possible I mucked up something when adding the MMR V1/V2 split. Was also not quite comfortable with it anyway since it seemed that methods were added and then the version was only bumped afterwards - this could quite possibly just be the update schedule followed when these were adjusted.

Indeed - I post-bumped the versions here paritytech/substrate#13509 after your comment regarding this (#5479 (comment)).

I acknowledge this compromise unfortunately leaves us with the inconsistency that nodes after ~v0.9.31 (iirc, the release where the first of these new/changed methods were introduced) but prior to v0.9.40 (where the version bump occurred) will report their API as being V1. Not bumping the API version would have avoided that, but begotten issues for earlier releases, hence the compromise.

Some small linting fixes suggested (as per CI failures) - there is however still an ordering issue, couldn't do that is the GH interface without mucking up.

Should be fixed now, if you rerun the workflow.

Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Thank you.

(Will be included in the next release over the weekend)

@polkadot-js-bot
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mmr_generateProof params is not correct
3 participants