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

Remove runtime APIs that are simply returning storage values #1246

Closed
svyatonik opened this issue Dec 9, 2021 · 2 comments · Fixed by #1348
Closed

Remove runtime APIs that are simply returning storage values #1246

svyatonik opened this issue Dec 9, 2021 · 2 comments · Fixed by #1348
Assignees
Milestone

Comments

@svyatonik
Copy link
Contributor

When this project was started, I/we had an impression that we shall no access storage values directly and instead call runtime APIs for that. That's because reading from storage imples that the client (mostly relay, but now also pallets sometimes) needs to compute storage key itself. So now we have runtime APIs like fn latest_generated_nonce(), fn latest_received_nonce(), ... that are just read some storage value and return it.

Today accessing storage directly is a common practice in our relays and pallets. And we have these storage-key computation functions in bp-runtime. The idea is to remove such simple runtime APIs - this will significantly decrease LOCs (both in relay and runtime code) and also will make implementing relay clients easier.

@svyatonik svyatonik added this to the Nice To Have milestone Dec 9, 2021
@svyatonik
Copy link
Contributor Author

svyatonik commented Dec 9, 2021

Submitter warning: this is really a good first issue, but it would require a lot of code changes.

UPD: actually, it seems impossible to continue parachains messages bridge without that. That's because parachain heads are opaque to the parachains finality module (probably this decision is wrong, but all parachains modules in polkadot are treating parachain heads as opaque byte vectors), but for our message/on-demand relays we need it to be decoded. The solution I see is to isolate decoding in relay => existing relays need to be refactored to support that. So I'm probably going to do that first (or maybe not 😕 )

UPD2: I'm going to implement first version of parachain messaging without it, but it is definitely be one of the first PRs in queue after that.

@svyatonik
Copy link
Contributor Author

There are 3 methods left in the runtime:

  1. <Chain>FinalityApi::best_finalized - I'm leaving it, because it is a perfect solution for hiding finality pallet details. Relayer (or other apps) may used this method without knowledge of which pallet is used for finality - pallet_bridge_grandpa, pallet_bridge_parachains or pallet_bridge_mmr;
  2. To<Chain>OutboundLaneApi::estimate_message_delivery_and_dispatch_fee - since logic of fee estimation is quite complex, it is better to use existing implementation, where fee is actually checked (in runtime), than do the same in every runtime client;
  3. To<Chain>OutboundLaneApi::message_details - this is another potential client for removal. But given Refactor existing deployments to use lane-specific payload #1317, I think it is better to leave relayer (and possibly other apps) unaware of what is actually encoded as payload. So I left it as-is for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant