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

Use Extensions to register offchain worker custom extensions #1719

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Sep 26, 2023

Closes #1671

As discovered correctly in that issue, custom externality extensions passed in from the outside as Box<dyn Extension> have some limitations. We store them in Extensions were the key is a TypeId, so multiple extensions are overriding each other.

I think the solution proposed in the issue is reasonable, so I changed the return type of the custom_extensions closure to return Extensions. They can then be passed to the runtime instance and merged with the existing extensions.

@skunert skunert added the T0-node This PR/Issue is related to the topic “node”. label Sep 26, 2023
@skunert skunert requested a review from a team September 26, 2023 17:51
@davxy
Copy link
Member

davxy commented Sep 26, 2023

Just an idea, but what about adding a method to the Extension trait to return an identifier here. (i.e. a method like get_id() -> ExtensionId with ExtensionId something like const [u8; N]).

And then use that id to populate the map

https://github.com/skunert/polkadot-sdk/blob/bce7c2465cfa11cdf51f0160fafecc7ef02e5a75/substrate/primitives/externalities/src/extensions.rs#L161-L162

For sure using the TypeId is more easy to use and less prone to collision (if not in this case where using the Box<dyn Extension>. But if the trait implementors are careful enough creating their unique ids maybe this could be a viable solution

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

We should probably remove the Any supertrait from Extension. Then we can forward the type_id function of Box<dyn Extension to the wrapped type. This would ensure that we don't take the TypeId of Box<Extension>. This should then probably fix this issue without requiring any of the changes in this pr.

@skunert
Copy link
Contributor Author

skunert commented Sep 27, 2023

Good suggestions guys, code is now simpler 👍

@skunert skunert merged commit 4902db2 into paritytech:master Sep 29, 2023
106 checks passed
ordian added a commit that referenced this pull request Oct 3, 2023
* master: (24 commits)
  Init System Parachain storage versions and add migration check jobs to CI (#1344)
  no-bound derives: Use absolute path for `core` (#1763)
  migrate alliance, fast-unstake and bags list to use derive-impl (#1636)
  Tvl pool staking (#1322)
  improve service error (#1734)
  frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717)
  Point documentation links to monorepo (#1741)
  [NPoS] Fix for Reward Deficit in the pool (#1255)
  Move import queue from `ChainSync` to `SyncingEngine` (#1736)
  Enable mocking contracts (#1331)
  Revert "fix(review-bot): pull secrets from `master` environment" (#1748)
  Remove kusama and polkadot runtime crates (#1731)
  Use `Extensions` to register offchain worker custom extensions (#1719)
  [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666)
  fix(review-bot): pull secrets from `master` environment (#1745)
  Fix `subkey inspect` output text padding (#1744)
  archive: Implement height, hashByHeight and call (#1582)
  rpc/client: Propagate `rpc_methods` method to reported methods (#1713)
  rococo-runtime: `RococoGenesisExt` removed (#1490)
  PVF: more filesystem sandboxing (#1373)
  ...
ordian added a commit that referenced this pull request Oct 10, 2023
* tsv-disabling-node-side: (24 commits)
  Init System Parachain storage versions and add migration check jobs to CI (#1344)
  no-bound derives: Use absolute path for `core` (#1763)
  migrate alliance, fast-unstake and bags list to use derive-impl (#1636)
  Tvl pool staking (#1322)
  improve service error (#1734)
  frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717)
  Point documentation links to monorepo (#1741)
  [NPoS] Fix for Reward Deficit in the pool (#1255)
  Move import queue from `ChainSync` to `SyncingEngine` (#1736)
  Enable mocking contracts (#1331)
  Revert "fix(review-bot): pull secrets from `master` environment" (#1748)
  Remove kusama and polkadot runtime crates (#1731)
  Use `Extensions` to register offchain worker custom extensions (#1719)
  [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666)
  fix(review-bot): pull secrets from `master` environment (#1745)
  Fix `subkey inspect` output text padding (#1744)
  archive: Implement height, hashByHeight and call (#1582)
  rpc/client: Propagate `rpc_methods` method to reported methods (#1713)
  rococo-runtime: `RococoGenesisExt` removed (#1490)
  PVF: more filesystem sandboxing (#1373)
  ...
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…ytech#1719)

Closes paritytech#1671 

Adds a `type_id` function to the `Extension` trait, allowing to properly store an retrieve
boxed `Extensions`.

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

Offchain worker custom extension not working
4 participants