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 Chain Extension docs #307

Merged
merged 8 commits into from
Feb 6, 2024
Merged

Update Chain Extension docs #307

merged 8 commits into from
Feb 6, 2024

Conversation

SkymanOne
Copy link
Contributor

Closes #303

  • Adds guide for declaration of multiple chain extensions
  • Adds guide for mocking chain extensions
  • Update the previous guide reflecting changes in the API

| `ink(handle_status = flag: bool)` | Optional | `true` | Assumes that the returned status code of the chain extension method always indicates success and therefore always loads and decodes the output buffer of the call. |

As with all ink! attributes multiple of them can either appear in a contiguous list:

```rust
type Access = i32;

#[ink::chain_extension]
#[ink::chain_extension(extension = 12)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have to consider this for the migration guide? Can you please take a look at the migration guide for chain extensions and see if there's anything to add/change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the notice in migration guide

```

:::note
The ink! repository contains the [full example](https://github.com/paritytech/ink/tree/master/integration-tests/combined-extension) illustrating how to combine existing chain extensions
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update this link to ink-examples once we've merged Sebastians PR there.

@SkymanOne SkymanOne requested a review from cmichi January 31, 2024 13:17
A migration in most cases should just be to rename `#[ink(extension = …)]` to
`#[ink(function = …)]`.
`#[ink(function = …)]`, and specifying `extension` argument in top level macro.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it's a migration guide: how can people find out the number to put into extension? What was "the default" ink! used in 4.x?

Copy link
Contributor Author

@SkymanOne SkymanOne Feb 1, 2024

Choose a reason for hiding this comment

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

I honestly don't know, but my guess it is 1 or it was ignored, since if the chain extension is not used in a tuple then there is no requirement to specify its ID

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, please try finding it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I tested, the ID can be anything

@SkymanOne SkymanOne requested a review from cmichi February 1, 2024 17:15
@@ -403,8 +403,21 @@ The argument type changed from `u32` to `u16`:
+Function,
```

The top level macro now accepts (extension = N: u16) argument to support multiple chain extensions.
If you are using only one extension, the ID can be any `u16` number,
otherwise please consult [`#[ink::chain_extension]` page](https://use.ink/5.x/macros-attributes/chain-extension)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
otherwise please consult [`#[ink::chain_extension]` page](https://use.ink/5.x/macros-attributes/chain-extension)
otherwise please consult the [`#[ink::chain_extension]` macro documentation](https://use.ink/5.x/macros-attributes/chain-extension)

Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if

  • someone uses only one chain extension in their contract right now
  • they put arbitrarily 1 there as part of the migration to 5.0
  • the chain adds another chain extension in its runtime with 1?

Can we come up with a recommendation of what number people should put there if they right now use only one extension?

At https://paritytech.github.io/polkadot-sdk/master/pallet_contracts/chain_extension/index.html#using-multiple-chain-extensions it says:

Chain specific extensions must use the reserved ID = 0 so that they can’t be registered with the registry.

Thinking out loud, a sensible recommendation could be to

  • consult the chain registry repo if they are using something like XCM or pallet-assets
  • consult the target chain documentation (Astar, etc.) regarding chain extensions and see if they say anything about the id.
  • check the runtime configuration of the target chain.
  • If no info is found in any of those places put 0 there?

Do you have an idea what the chain extension id for PSP-22 and the other PSP's would be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will break compatibility for the scenario you described. Consulting the target chain's docs is the most sensible approach, as the chain registry repo appears to be unmaintained (the last changes were 2 years ago).

Until then, if you are sure that the chain only uses a single chain, then putting any number. is fine

Co-authored-by: Michael Müller <[email protected]>
@SkymanOne SkymanOne requested a review from cmichi February 2, 2024 17:22
@cmichi cmichi merged commit 4dea70d into master Feb 6, 2024
@cmichi cmichi deleted the gn/ce-update branch February 6, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework page on chain extensions to include changes for supporting multiple chain extensions
2 participants