Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

seal: Rework the API #6355

Closed
2 of 3 tasks
athei opened this issue Jun 15, 2020 · 0 comments · Fixed by #6573
Closed
2 of 3 tasks

seal: Rework the API #6355

athei opened this issue Jun 15, 2020 · 0 comments · Fixed by #6573
Assignees
Labels
I7-refactor Code needs refactoring.

Comments

@athei
Copy link
Member

athei commented Jun 15, 2020

This issue tracks all changes to the Seal API deemed necessary before deploying it.

Part 1

ext_gas_price should take the gas amount as argument

There is no longer something like gas. We merely keep the term around for historical reasons. Costs are measured in weight. Currently, we provide gas price as the costs for one weight. Calculating the costs for a single weight is subject to rounding errors. Therefore we should change the API so that the caller has to input the amount of weight for that the costs should be calculated:

ext_gas_price(u64) -> u128

Removal of of unused questionable API

We want to only provide a basic set of APIs because once seal is deployed we cannot remove them. We can always add API later when necessary. Additionally we plan to allow runtime authors to add functions to the API with a mechanism we call "ChainExtensions" without forking the seal pallet.

The following APIs are scheduled for removal:

ext_dispatch_call

This is difficult to audit as it can make arbitrary calls into the runtime. For that reason we do not want it to exist in our first deployment. In contrast to regular extrinsic dispatch we do not spawn a new runtime instance for each call executed in this way. This makes this more difficult to reason about. A good example would be a sequence of dispatches that DoS the memory allocator of the runtime.

get_runtime_storage

Rather than having a blanket way of retrieving values from runtime storage we want runtime authors to make use of an upcoming mechanism called "Chain Extensions". This will allow runtime authors to augment the API with their own functions that can supply contracts with information specific to their chain.

Part 2

Remove the Scratch Buffer

We want to re-design the current contracts API to no longer make use of the scratch buffer. The motivation and current situation is described here: #5513 The linked issue is a subset of what we are trying to accomplish. We already reached consensus on that subset.

However, after a longer discussion with @pepyakin we came to the conclusion that the complete removal of the scratch buffer might be viable. What follows is the rationale for that proposal. We hope for feedback to determine whether this is the right path forward.

The draw back of a (pointer, size) API is that it can fail when the caller supplies a too small buffer to fit the result in. The main contention point on the topic of complete scratch buffer are API calls that are in itself expensive to repeat on error (accessing trie storage) or impossible to repeat without side effects (calling other contracts).

The argumentation here is that this error case should be an exceptional state that we should not optimize for. The reasoning for that is that either:

  1. The contract knows the max size of the expected result because it is defined by code.
  2. The size is dynamic because it might be some kind of proxy contract. Then the size is passed in as argument by the caller.

As a matter of fact the user always had to make this estimation by supplying the gas_limit as reading the result is payed for per byte. With the exception of allowing a contract to consume all gas. We do not consider that a safe practice. Therefore it should not be optimized for.

Contract languages (like ink!) should make sure that the error case stays exceptional. These high level languages would hide the size argument from the user wherever they can (most cases). For more complicated cases like calling another contract this can be achieved by including the max return value size for every contract message in the Metadata/ABI.

Another shot at this topic was made here: #3503 . However, that issue requested a much bigger change and also included provisions to make multiple calls of the same API call efficient. We merely propose keeping the current API as is but replace the scratch buffer by a (pointer, size) argument. We can do that because we declare that calling an API multiple times is an exceptional case for which we do not optimize.

We are mainly interested in legit use cases where 1. or 2. does not apply. That means where querying the size of the result buffer in a dynamic fashion is necessary (it is the job of high level languages to provide a "nice" abstraction).

Allow contract instantiation to return data

Currently, when calling ext_instantiate any data that is returned from the deployment function is discarded. With the removal of the scratch buffer as described above we can just add an additional (ptr, len) tuple to the ext_instantiateAPI in order to allow callers to optionally copy data returned by the deploy to their own linear memory.

Re-Introduction of ext_input

Since there is no-longer a notion of scratch buffer we need an API to retrieve the input to a contract call. This function copies the input data to contract memory and can only be called once per contract call.

Do not pass status code to callers

Currently, the i32 return value of call and deploy is passed to the caller (meaning the caller of ext_call and ext_instantiate). However, the output buffer as opaque block of data is already passed between contracts and should be used by the hosted contracts independently of the supervisor. Seal should not be involved in error handling of the contracts so each language can develop its own approach in order to allow innovation. Therefore we make the following changes.

  • Change the return type of deploy and call to ()
  • Change ext_return so that success can be indicated to seal ext_return(flags: u32, output: Vec<u8>)
  • No user defined status code is returned from ext_call and ext_instantiate any longer. It merely returns whether the execution did trap, was reverted by user request or did succeed.

This also changes the u8 status_code to a u32 flags field. Since those are also returned via RPC adjustments to those users are necessary (polkadot js api).

Unified Return Type

The return values of all API functions are now defined by a common enum to make it easier to look up all the different errors that can occur.

Rename ext_gas_price to ext_weight_to_fee

@athei athei added the I7-refactor Code needs refactoring. label Jun 15, 2020
@athei athei self-assigned this Jun 15, 2020
@athei athei changed the title Contracts: Replace all scratch buffer with a (pointer, size) argument Contracts: Replace scratch buffer with a (pointer, size) argument Jun 15, 2020
@athei athei changed the title Contracts: Replace scratch buffer with a (pointer, size) argument SEAL: Rework the API Jun 15, 2020
@athei athei changed the title SEAL: Rework the API Seal: Rework the API Jun 16, 2020
@pepyakin pepyakin changed the title Seal: Rework the API seal: Rework the API Jul 1, 2020
@ghost ghost closed this as completed in #6573 Jul 9, 2020
@athei athei moved this to Done in Smart Contracts Jul 27, 2022
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I7-refactor Code needs refactoring.
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

1 participant