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

refactor: generic extension #218

Merged
merged 56 commits into from
Sep 6, 2024
Merged

refactor: generic extension #218

merged 56 commits into from
Sep 6, 2024

Conversation

evilrobot-01
Copy link
Collaborator

@evilrobot-01 evilrobot-01 commented Aug 19, 2024

Refactors the chain extension to maximise flexibility, ensuring that it is highly configurable and so that anything which is runtime specific is defined there instead - e.g. versioning.

A high level overview of the components:

[contract environment: contract ⇄ pop-api] ⇄ [runtime: pallet-contractspop-chain-extensionpallet-apifungibles]

The simplest analogy is that of a pipeline, where a request arrives and moves through a pipeline where it can be versioned (processed) based on other information available from the environment and then decoded into a valid runtime call. pop-api chooses to encode the version within the ext_id/func_id for example. The reverse also then allows for versioning by adapting any result (read) from a pallet or error into a type/shape the applies to the specific version. More specifics below.

Errors

The extension supports conversion of a DispatchError into some other error type. This is to allow a pop-api specific error type which is also versionable, which in turn is converted into a u32 status code used by the contract. The contract can then optionally convert this status code back into a specific versioned error type if desired, with some contract size overhead.

Reads

The results of reads also need to be versionable, so the ReadState function allows an additional conversion to be configured. This is so that we can simply map a current read result type from a pallet into some prior versioned result type before encoding it into the bytes expected by the contract based on the version requested.

Versioning

Versioning is considered a runtime-specific requirement and all related logic is therefore contained within the versioning module within the runtime. The generic chain extension is configured to be version aware by the various types that are plugged into it.

TODOs

  • unit tests

Closes #202

Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

This fulfils all the needs that we have identified and well done with the amount of flexibility this implementation provides!

In the past we've experienced how useful this flexibility can be so very much onboard.

extension/src/reboot.rs Outdated Show resolved Hide resolved
extension/src/reboot.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

Looking good to proceed. We might be able to not use any additional error variant other than Other with your proposed change to using the pallet contracts error for NoChainExtensionId. This prevents us wanting to delete it in the future.

extension/src/functions.rs Outdated Show resolved Hide resolved
extension/src/lib.rs Show resolved Hide resolved
extension/src/functions.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 85.49874% with 173 lines in your changes missing coverage. Please review.

Please upload report for BASE (daan/api@8d1c085). Learn more about missing BASE report.

Files with missing lines Patch % Lines
runtime/devnet/src/config/api/versioning.rs 60.00% 52 Missing ⚠️
pallets/api/src/extension.rs 48.33% 30 Missing and 1 partial ⚠️
runtime/devnet/src/config/api/mod.rs 35.29% 22 Missing ⚠️
extension/src/functions.rs 92.50% 0 Missing and 20 partials ⚠️
primitives/src/lib.rs 0.00% 16 Missing ⚠️
extension/src/lib.rs 93.87% 0 Missing and 6 partials ⚠️
extension/src/mock.rs 95.12% 5 Missing and 1 partial ⚠️
extension/src/decoding.rs 96.91% 1 Missing and 4 partials ⚠️
extension/src/tests.rs 97.07% 0 Missing and 5 partials ⚠️
pallets/api/src/fungibles/mod.rs 88.23% 4 Missing ⚠️
... and 3 more
@@             Coverage Diff             @@
##             daan/api     #218   +/-   ##
===========================================
  Coverage            ?   46.23%           
===========================================
  Files               ?       40           
  Lines               ?     3707           
  Branches            ?     3707           
===========================================
  Hits                ?     1714           
  Misses              ?     1938           
  Partials            ?       55           
Files with missing lines Coverage Δ
extension/src/environment.rs 100.00% <100.00%> (ø)
extension/src/matching.rs 100.00% <100.00%> (ø)
pallets/api/src/fungibles/tests.rs 100.00% <100.00%> (ø)
runtime/devnet/src/config/contracts.rs 60.00% <ø> (ø)
runtime/devnet/src/config/proxy.rs 0.00% <ø> (ø)
runtime/devnet/src/lib.rs 17.66% <ø> (ø)
runtime/testnet/src/config/proxy.rs 0.00% <ø> (ø)
runtime/testnet/src/extensions.rs 0.00% <ø> (ø)
runtime/testnet/src/lib.rs 14.88% <ø> (ø)
node/src/command.rs 0.00% <0.00%> (ø)
... and 12 more

@evilrobot-01 evilrobot-01 marked this pull request as ready for review August 22, 2024 12:54
@evilrobot-01 evilrobot-01 linked an issue Aug 22, 2024 that may be closed by this pull request
Copy link
Collaborator

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

Really clean PR! The new format is great. Left a nitpicks.
Will review again after unit tests are added.

pallets/api/src/extension.rs Outdated Show resolved Hide resolved
extension/src/functions.rs Outdated Show resolved Hide resolved
runtime/devnet/src/config/api/versioning.rs Outdated Show resolved Hide resolved
extension/src/lib.rs Outdated Show resolved Hide resolved
pallets/api/src/extension.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@chungquantin chungquantin left a comment

Choose a reason for hiding this comment

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

Mindblowing!

extension/src/decoding.rs Outdated Show resolved Hide resolved
extension/src/lib.rs Outdated Show resolved Hide resolved
@evilrobot-01
Copy link
Collaborator Author

@peterwht @chungquantin as per your comments above, I addressed them in 2152867 and resolved the conversation. Please could you kindly take a look at the commit and let me know if any concerns?

@chungquantin
Copy link
Collaborator

@Daanvdplas
Copy link
Collaborator

As for the open questions regarding whether to convert the DispatchError in e.g. the ReadState function. My view is that in the end it is also a pallet with an index. We do account for changes in a pallet index, so why not include those as well? I think treating all pallets the same is the easiest and safest.

This was referenced Aug 30, 2024
Copy link
Collaborator

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

Reviewed again. The architecture is really awesome. Left a few small comments.

extension/src/environment.rs Show resolved Hide resolved
extension/src/lib.rs Outdated Show resolved Hide resolved
extension/src/decoding.rs Outdated Show resolved Hide resolved
pallets/api/src/extension.rs Outdated Show resolved Hide resolved
pallets/api/src/extension.rs Outdated Show resolved Hide resolved
pallets/api/src/extension.rs Outdated Show resolved Hide resolved
pallets/api/src/extension.rs Outdated Show resolved Hide resolved
runtime/devnet/src/config/api/versioning.rs Outdated Show resolved Hide resolved
runtime/devnet/src/config/api/versioning.rs Show resolved Hide resolved
@chungquantin chungquantin self-requested a review September 6, 2024 09:06
@evilrobot-01 evilrobot-01 merged commit 479dbb0 into daan/api Sep 6, 2024
10 checks passed
@evilrobot-01 evilrobot-01 deleted the frank/extension branch September 6, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: generic extension
5 participants