r0gue-io / pop-node

Pop Network makes it easy for smart contract developers to use the Power of Polkadot.
The Unlicense
24 stars 6 forks source link

refactor (pop api): match chain extension functions to standard. #94

Closed Daanvdplas closed 3 months ago

Daanvdplas commented 4 months ago

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:

evilrobot-01 commented 3 months ago

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 commented 3 months ago

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

Daanvdplas commented 3 months ago

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 commented 3 months ago

Why ERC-20 over PSP22?

evilrobot-01 commented 3 months ago

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 commented 3 months ago

Implemented in #132