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 (pop api): match chain extension functions to standard. #94

Closed
4 tasks done
Tracked by #132
Daanvdplas opened this issue Jul 5, 2024 · 6 comments
Closed
4 tasks done
Tracked by #132

Comments

@Daanvdplas
Copy link
Collaborator

Daanvdplas commented Jul 5, 2024

I stumbled upon a blocker implementing the (ERC20) approve. It sets the allowance value in stead of working additive like pallet assets’ approve_transfer. There is a difference in logic, causing to call two (for decrease_allowance even three) runtime functions back and forth (from contract to runtime to contract to runtime), which doesn’t go without costs because going from contract to runtime and vice versa has overhead and will add to the total weight. I think we should have the chain extension functions adhere to the standards which removes this requirement.

A contract (aka the pop api ink! library) should be as light as possible - it should only contain logic very specific to the contract*. Currently each contract will suffer the pain of the standard conflicts (between polkadot sdk and ERC standards). This must be handled in the runtime. This ensures that contracts using the api remain specialized, lightweight, and cost-effective while using the api. In addition, what if pallet assets' adds the decrease_allowance functionality in the pallet? With the current design the contract is stuck with interacting 3 times back and forth, with the new design, the runtime can simply upgrade the chain extension functionality on the runtime.

Open question: This redesign will require to not strictly call pallet functions but also standard functions. This won't allow us to communicate with the runtime via [version, function_id, pallet_index, dispatchable_index] (we encode the following and send it to the runtime for it to know what to do). Easiest solution would be to change the dispatchable_index to chain_extension_function.

First #71 should be merged

*we have two contracts. One 4 x bigger than the other in size. Both provide the same contract message. Calling this message on the bigger contract will be more expensive then the smaller.

Notes:

  • rename assets feature in pop api and primitives to fungibles.
  • use enum for keys to read state.
  • the calling of the chain extension can be refactored, inspiration can be found here
  • add docs to all public api functions.
@evilrobot-01
Copy link
Collaborator

Why does it need to upgrade the chain extension? Why cant the desired functionality be implemented in a pallet, which is specified in the contract side ink! library? The API is not limited to that which is included with the contract. Think of that as the interface of the API, with the implementation being some module within the runtime.

@Daanvdplas
Copy link
Collaborator Author

That is the way to do it. Thanks for reminding me.

@Daanvdplas
Copy link
Collaborator Author

Daanvdplas commented Jul 17, 2024

If the interface is based on a standard, do we need versioning for calling these interfaces? The interfaces should never change; parameters should never change, the implementation should also never change. This potentially means that we only use the version for the conversion of the error.

Another open question; do we need the function id? A use case pallet can have functions calling dispatchables but also read state functions.

@peterwht
Copy link
Collaborator

Why ERC-20 over PSP22?

@evilrobot-01
Copy link
Collaborator

If the interface is based on a standard, do we need versioning for calling these interfaces?

In this case probably not, but this won't be the only thing added to the first version. Contracts shouldn't be specifying the version, they should be using exports of the versioned API under the main namespace at the time of implementation. That way v1 just exports the functions which haven't changed since v0, and the contract just imports them without caring. Whilst the version is currently being encoded, that can easily be refactored out, as suggested by having reusable functions for encoding the chain extension method identifier.

Based on this, it's easier to just stick in the first version of the API rather than have some separate non-versioned bit. You never know what happen, so having the option is more valuable than not.

Another open question; do we need the function id? A use case pallet can have functions calling dispatchables but also read state functions.

Are these read state functions added to the runtimecall or similar enum with an index? In other words, are they addressable via a few bytes. The function id was just a way to signal whether it was runtime call or a state read, the latter to an enum to easily match in to charge the right weight and return the right result.

@Daanvdplas
Copy link
Collaborator Author

Implemented in #132

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

No branches or pull requests

3 participants