Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet-mmr: Consolidate/deduplicate methods in runtime API and RPC API #12391

Closed
serban300 opened this issue Sep 30, 2022 · 3 comments · Fixed by #12530
Closed

pallet-mmr: Consolidate/deduplicate methods in runtime API and RPC API #12391

serban300 opened this issue Sep 30, 2022 · 3 comments · Fixed by #12530
Assignees
Labels
I7-refactor Code needs refactoring. U2-some_time_soon Issue is worth doing soon.

Comments

@serban300
Copy link
Contributor

Since we're changing the pallet-mmr runtime & RPC APIs as part of #12339 it's a good opportunity to revisit some other decisions related to the API:

  1. Whether we should have 2 separate methods generate_batch_proof() and generate_historical_batch_proof(). Since in the end generate_batch_proof() is a special case of generate_historical_batch_proof(). See the discussion here: pallet-mmr: generate historical proofs #12324 (comment)
  2. If we should keep generate_proof(), verify_proof(), and verify_proof_stateless() since all these can be done via batching methods (e.g. generate_batch_proof(), etc)

cc: @acatangiu @Lederstrumpf @svyatonik

@acatangiu
Copy link
Contributor

acatangiu commented Oct 3, 2022

I agree with unifying all, but drop "batching" or "historical" from new "universal" API name, should be simple generate_proof(), verify_proof(), and verify_proof_stateless() with optional extra params.
Documentation should also be very clear to explain specific param combinations.

@acatangiu acatangiu moved this to Need for Kusama 🗒 in BEEFY Oct 3, 2022
@acatangiu acatangiu added I7-refactor Code needs refactoring. U2-some_time_soon Issue is worth doing soon. labels Oct 3, 2022
@Lederstrumpf
Copy link
Contributor

I agree with unifying all, but drop "batching" or "historical" from new "universal" API name, should be simple generate_proof(), verify_proof(), and verify_proof_stateless() with optional extra params.
Documentation should also be very clear to explain specific param combinations.

ditto: I support unification, and if unified, then batching & historical qualifiers are superfluous and should be dropped.

@serban300
Copy link
Contributor Author

serban300 commented Oct 3, 2022

Sounds good ! I also agree with this naming scheme.

@acatangiu acatangiu changed the title pallet-mmr: Discuss reducing the number of methods in runtime API and RPC API pallet-mmr: Consolidate/deduplicate methods in runtime API and RPC API Oct 3, 2022
Repository owner moved this from Need for Kusama 🗒 to Done ✅ in BEEFY Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I7-refactor Code needs refactoring. U2-some_time_soon Issue is worth doing soon.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants