Update contract' gas to Weight type #5324

jasl · 2022-11-08T17:05:19Z

I don't have confidence I'm doing right.

Starting from polkadot v0.9.31, the gas relates fields has changed to Weight (V2, https://github.com/paritytech/substrate/blob/polkadot-v0.9.31/frame/contracts/primitives/src/lib.rs#L35-L71 ),
but it looks polkadot.js still define them u64

packages/api-contract/src/types.ts

statictype · 2022-11-09T10:21:05Z

this is great! thanks @jasl
the runtime call needs to be adjusted too. similar to exec

jasl · 2022-11-09T10:43:59Z

Make Contracts Runtime Calls support WeightV2

I make a new commit for this, check 3bbb6c0

jasl · 2022-11-09T10:45:21Z

@jacogr what does the Gas type definition do? should I convert all gas relates fields (gasLimit, gasRequired) to Gas type?

jasl · 2022-11-09T11:03:59Z

ops it seems I force pushed too many times the CI refuse to run ...
unforuntately my local environment fails on some random parts...

statictype · 2022-11-09T11:25:20Z

no worrries. the CI doesn't run automatically for first time contributions. it's still a bit more work, unfortunately.
i'm testing your changes locally with apps.
@jacogr is this necessary? Can we return gasRequired as is?

jasl · 2022-11-09T12:40:07Z

https://github.com/polkadot-js/api/pull/5324/files#diff-e923378b0f9524ff3bcbc37be8010d0f81c271e98fceabc6cc9641adb08de09bR58
the Weight here, are pointed to WeightV1
https://github.com/polkadot-js/api/blob/master/packages/types/src/interfaces/runtime/types.ts#L392
so that's why CI failed because
error: TS2345 [ERROR]: Argument of type '{ refTime: BN; }' is not assignable to parameter of type 'AnyNumber | u64'.
I guess

statictype · 2022-11-09T12:50:07Z

@jasl in order for the type definitions for runtime calls to be generated you need to update gasLimit in this file and run yarn build. i think your last commit is unnecessary.

jasl · 2022-11-09T13:07:58Z

this file

understood, thank you, now the latest commit should pass yarn build without error

jasl · 2022-11-09T13:17:08Z

CI PASSED !!!

packages/api-contract/src/types.ts

athei

The Weight is optional in runtime APIs now. It allows you to let the runtime choose a default value in case you don't care (you usually don't care if you use it to estimate gas_required).

athei · 2022-11-10T10:45:28Z

packages/types/src/interfaces/contracts/runtime.ts

@@ -24,7 +24,7 @@ export const runtime: DefinitionsCall = {
            },
            {
              name: 'gasLimit',
-              type: 'u64'
+              type: 'Weight'


This is an option: https://github.com/paritytech/substrate/blob/78f8f708781da6fd340ef2a1fc288060a35295ad/frame/contracts/src/lib.rs#L1215

Changed to Option<Weight>

athei · 2022-11-10T10:45:55Z

packages/types/src/interfaces/contracts/runtime.ts

@@ -64,7 +64,7 @@ export const runtime: DefinitionsCall = {
            },
            {
              name: 'gasLimit',
-              type: 'u64'
+              type: 'Weight'


This is an option: https://github.com/paritytech/substrate/blob/78f8f708781da6fd340ef2a1fc288060a35295ad/frame/contracts/src/lib.rs#L1226

Changed to Option<Weight>

athei · 2022-11-10T10:48:32Z

packages/types/src/interfaces/contracts/definitions.ts

@@ -45,7 +45,7 @@ export default {
      origin: 'AccountId',
      dest: 'AccountId',
      value: 'Balance',
-      gasLimit: 'u64',
+      gasLimit: 'Weight',


Is optional.

Changed to Option<Weight>

jasl · 2022-11-10T11:05:00Z

The Weight is optional in runtime APIs now. It allows you to let the runtime choose a default value in case you don't care (you usually don't care if you use it to estimate gas_required).

Changed your mentioned fields to Option<Weight>

athei · 2022-11-10T11:16:44Z

Don't you also need to adapt some code in the contracts api that uses those, now changed, signatures?

jasl · 2022-11-10T11:19:31Z

Don't you also need to adapt some code in the contracts api that uses those, now changed, signatures?

Sorry, I'm not familier with polkadot-js codebase, but if could give me some directions, I think I can work out

jasl · 2022-11-10T11:33:38Z

I do unit test in local, they're all passed, you mean I need to add unit tests for WeightV2?

jacogr

I am literally not sure about these Option<Weight> changes - there seems to have been a breaking update to the runtime interfaces without a version update. This is hugely problematic, there is a reason why the runtime interfaces have versions.

EDIT: Indeed. This PR broke the world without updating the runtime api version. paritytech/substrate#12429 - not sure how that passed reviews. It is creating huge (unsolvable) issues here now.

jacogr · 2022-11-10T11:46:24Z

packages/api-contract/src/types.ts

@@ -55,8 +55,8 @@ export interface InterfaceContractCalls {

 export interface ContractCallOutcome {
  debugMessage: Text;
-  gasConsumed: u64;
-  gasRequired: u64;
+  gasConsumed: u64 | WeightV2;


This should just be Weight - this is a bit tricky, but the runtime will inject the correct type.

jacogr · 2022-11-10T11:46:28Z

packages/types/src/interfaces/contracts/definitions.ts

@@ -45,7 +45,7 @@ export default {
      origin: 'AccountId',
      dest: 'AccountId',
      value: 'Balance',
-      gasLimit: 'u64',
+      gasLimit: 'Option<Weight>',


This is going to be problematic - there is now old and new, we need to cater for both.

does u64 | Option<Weight> work?

For his one we need a runtime result for V1 as well - that keeps as it. For v2, we have a new version that gets returned. So basically rename the existing to ContractCallRequestV1 (to be returned for v1 runtime) and the change the above version for Option<Weight>.

Basically (until we finally have all this info on the metadata), we need to have old and new types available. Yes, it is a bitch.

I already created ContractCallRequestV1, on line 44

jacogr · 2022-11-10T11:46:31Z

packages/types/src/interfaces/contracts/runtime.ts

@@ -24,7 +24,7 @@ export const runtime: DefinitionsCall = {
            },
            {
              name: 'gasLimit',
-              type: 'u64'
+              type: 'Option<Weight>'


This can't be - the runtime is supposed to be versioned, i.e. there should not be a breaking change without updating versions. (This is the same as above, this is breaking for users)

EDIT: See my comment with the problematic PR linked - no idea how to solve this at all.

IMO, Option<Weight> can be treated as 0u64? and 0u64 can be treated as None ?

There is a difference in SCALE encoding Option<u64> vs u64. The former would be 9 bytes in length, the later would be 8 bytes. So it breaks the structure decoding if wrong.

jasl · 2022-11-10T12:16:28Z

EDIT: Indeed. This PR broke the world without updating the runtime api version. paritytech/substrate#12429 - not sure how that passed reviews. It is creating huge (unsolvable) issues here now.

Do you have idea @athei ?

athei · 2022-11-10T13:14:41Z

EDIT: Indeed. This PR broke the world without updating the runtime api version. paritytech/substrate#12429 - not sure how that passed reviews. It is creating huge (unsolvable) issues here now.

What exactly do you mean by "broke the world"? Do you mean existing code which didn't update to a new polkadot.js version or is new code also affected?

jacogr · 2022-11-10T13:21:06Z

The issue is that the state_call interface has been changed without updating the runtime api version via decl_api. The JS API has no way to detect which version is in use - generally (with all other runtime APIs), the runtime api version gives this indication, i.e. for version 1 (the default if not specified), use X, for version 2, use Y.

This is the great thing about using state_call, it is versioned, unlike RPCs which are always problematic in terms of params and results.

The nightmare here is that the result encoding has completely changed - since none of this information is in the metadata (there is a Substrate issue to expose it), there is no way to know how to decode it, i.e. what is the format. This would have been solved with adding the version 2 attribute to the APIs.

So atm - the runtime calls for V1 will work against older deployed chains. If they upgrade to this new v1 interface, it cannot be decoded. If the definitions are changed to match the "new v1 interfaces" the results from deployed "old v1" interfaces will break. So some chains will break since we just don't know.

Hence - "breaking the world", since somebody is going to be unhappy with 2-same-versioned-yet-different state_call interfaces.

jacogr · 2022-11-10T13:35:44Z

Here is an example of a change made to the result of a runtime api declaration where the version was bumped, https://github.com/paritytech/substrate/blob/38a955ba4a0c967659e6c944c56c664fce0f8f23/primitives/consensus/babe/src/lib.rs#L363-L372

With the above, anybody can always know which one of these interfaces are in operation - so either can be used (and is defined as such). This is all based on the info returned from the state_getRuntimeVersion RPC which returns which version of the runtime API is used. (This is returned on a per API basis)

So literally what is missing is the #[api_version(2)] annotation to indicate a version change here. Without it, there are 2 incompatible v1 interfaces.

athei · 2022-11-10T13:39:10Z

So in order to unblock this PR it would be fine if I just flag the new version as version 2 but don't supply a version 1 for backwards compatibility? This will still break old polkadot.js code (which isn't aware of a new version) but we are okayish with that?

The JS API has no way to detect which version is in use - generally (with all other runtime APIs), the runtime api version gives this indication, i.e. for version 1 (the default if not specified), use X, for version 2, use Y.

How do you detect the version (given that I add the version attribute in substrate)? There is no indication in metadata.

jasl · 2022-11-10T13:39:22Z

So we need to do these things:

add #[api_version(2)] to pallet_contracts RuntimeAPI
backport above change to polkadot-v0.9.31 and polkadot-v0.9.32 branch
refactor this PR use Weight for V2 API

polkadot-v0.9.31 release just few days ago, I think it's not late to append the fix.

Am I understanding correct?

jasl · 2022-11-11T08:17:53Z

Sorry I think I need more clear instructions, PR for only the v2 + v1 runtime changes do you mean I should only push
packages/types/src/interfaces/contracts/runtime.ts
and
packages/types/src/interfaces/contracts/types.ts
?

jasl · 2022-11-11T08:26:49Z

I rebased master

jasl · 2022-11-11T08:42:31Z

Try my best to understanding your means... I'm dizzy, sorry 😓

jacogr · 2022-11-11T09:49:20Z

yarn build (or yarn build:metadata) will only yield updated interfaces when the static Substrate metadata has been updated. Before that, the only known to the generation is basically a week old. It should generate for the Contract V2 APIs now after you pulled-in master which has current metadata.

(Basically on the generated types it uses the chain metadata - this means that all chains supply their metadata and it tweaks the generated types based on that)

TL;DR The joys of having statically generated types based on something dynamic like runtimes...

jasl · 2022-11-11T09:52:02Z

yarn build will only yield updated code when the static Substrate metadata has been updated. Before that, the only known to the generation is basically a week old. It should generate for the Contract V2 APIs now after you pulled-in master which has current metadata.

(Basically on the generated types it uses the chain metadata - this means that all chains supply their metadata and it tweaks the generated types based on that)

Yeah, I rebased master and specify deprecated Contracts RPC to use V1 struct, then yarn build, I did yarn test in my local it seems passed

jacogr · 2022-11-11T09:54:55Z

packages/rpc-augment/src/augment/jsonrpc.ts

@@ -145,7 +145,7 @@ declare module '@polkadot/rpc-core/types/jsonrpc' {
       * @deprecated Use the runtime interface `api.call.contractsApi.call` instead
       * Executes a call to a contract
       **/
-      call: AugmentedRpc<(callRequest: ContractCallRequest | { origin?: any; dest?: any; value?: any; gasLimit?: any; storageDepositLimit?: any; inputData?: any } | string | Uint8Array, at?: BlockHash | string | Uint8Array) => Observable<ContractExecResult>>;
+      call: AugmentedRpc<(callRequest: ContractCallRequestV1 | { origin?: any; dest?: any; value?: any; gasLimit?: any; storageDepositLimit?: any; inputData?: any } | string | Uint8Array, at?: BlockHash | string | Uint8Array) => Observable<ContractExecResult>>;


If you run yarn build:metadata now, it should actually pull in the latest V2 stuff (assuming there is a version: 2 definition available in contracts/runtime.ts)

I ran yarn build:metadata seems no file changes

Ensure you have master merged in - just did a quick (5 mins, literally), ContractsApi/2 in here and it generates the new interfaces - #5330

(No idea if it is fully correct in regards to ContractsApi definitions)

I rebased master and it has the new metadata commit...
I see you open a new PR for Contracts V2, do you wanna take it?

Happy if you take it - you have done the hard work. But either way ok. (Just wanted to show that the runtime apis does get updated, once defined, on master, in the above PR)

I believe your issues (reading below), is that your u64 -> Option changes are on the v1 APIs, which are not being generated. My PR adds for v2 (and keeps v1 as-is, with some small historic-types-now updates), hence being generated.

I miss the head of the file yarn polkadot-types-from-chain, so I should build a latest Substrate node, and run yarn polkadot-types-from-chain --endpoint ws://127.0.0.1:9944 --output packages/rpc-augment/src/augment/jsonrpc.ts

We do include the latest static metadata, so yarn build:metadata will pick it up. (Well, "latest" as of this morning - it gets updated once weekly)

I port your PR's changes, and rerun yarn build:metadata, Doe it look correct now?

jasl · 2022-11-11T19:26:33Z

Just made another batch of changes, ran yarn build and yarn build:metadata no new changes, yarn test pass

jacogr · 2022-11-12T15:08:26Z

Nice. I can see the v2 apis popping up now. Will get it in in the morning.

jasl · 2022-11-12T15:16:28Z

Nice. I can see the v2 apis popping up now. Will get it in in the morning.

Thank you for your patience mentoring, if possible, could you make a new release after this PR merged? our team is blocked by the issue

jacogr · 2022-11-12T15:35:37Z

Releases go on weekends. (Generally Sundays, each week, but I have been known to push on Saturday as well) The next is scheduled for tomorrow.

jacogr

Thank you. Seems spot-on.

polkadot-js-bot · 2022-11-15T10:28:41Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jacogr reviewed Nov 8, 2022

View reviewed changes

packages/api-contract/src/types.ts Outdated Show resolved Hide resolved

jasl force-pushed the try-fix-contracts branch 2 times, most recently from 4db1d78 to 856ecf3 Compare November 8, 2022 20:52

jacogr reviewed Nov 9, 2022

View reviewed changes

packages/api-contract/src/types.ts Outdated Show resolved Hide resolved

jasl force-pushed the try-fix-contracts branch from 3b5b29a to 856ecf3 Compare November 9, 2022 09:25

jasl force-pushed the try-fix-contracts branch 2 times, most recently from 5761c90 to 3bbb6c0 Compare November 9, 2022 11:00

jasl force-pushed the try-fix-contracts branch from d9b74f0 to 250db93 Compare November 9, 2022 12:50

statictype reviewed Nov 9, 2022

View reviewed changes

packages/api-contract/src/types.ts Outdated Show resolved Hide resolved

athei reviewed Nov 10, 2022

View reviewed changes

jacogr reviewed Nov 10, 2022

View reviewed changes

jasl added 7 commits November 11, 2022 16:21

Update contract' gas to Weight type

70087b1

Make Contracts Runtime Calls support WeightV2

05093af

fix gasConsumed & gasConsumed type

78258e6

Regenerate contracts types

4b0bcb7

Correct ContractCallOutcome

7647400

gasLimit are optional

4668785

Apply suggestion

1da7ffa

jasl force-pushed the try-fix-contracts branch from 29a3213 to 1da7ffa Compare November 11, 2022 08:26

Add ContractCallRequestV1 and specify old rpc use that

13e3828

jacogr reviewed Nov 11, 2022

View reviewed changes

jasl added 5 commits November 11, 2022 18:02

Add back getGas

5c0e8e1

Port jg-ContractsApi-v2 (polkadot-js#5330)

c9f605d

Run yarn build:metadata

5401b8a

There is no 'ContractCallRequestV1', only 'ContractCallRequest'

277b7d8

api.tx.contracts.call.meta no version field, _isOldWeight still work?

bcddda6

jacogr mentioned this pull request Nov 12, 2022

ContractsApi v2 support #5330

Closed

jacogr approved these changes Nov 13, 2022

View reviewed changes

jacogr merged commit cad2b0f into polkadot-js:master Nov 13, 2022

jasl deleted the try-fix-contracts branch November 13, 2022 09:10

polkadot-js locked as resolved and limited conversation to collaborators Nov 15, 2022

Update contract' gas to Weight type #5324

Update contract' gas to Weight type #5324

Conversation

jasl commented Nov 8, 2022 • edited Loading

statictype commented Nov 9, 2022

jasl commented Nov 9, 2022 • edited Loading

jasl commented Nov 9, 2022

jasl commented Nov 9, 2022

statictype commented Nov 9, 2022

jasl commented Nov 9, 2022

statictype commented Nov 9, 2022 • edited Loading

jasl commented Nov 9, 2022

jasl commented Nov 9, 2022

athei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasl commented Nov 10, 2022

athei commented Nov 10, 2022

jasl commented Nov 10, 2022

jasl commented Nov 10, 2022

jacogr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasl commented Nov 10, 2022

athei commented Nov 10, 2022

jacogr commented Nov 10, 2022 • edited Loading

jacogr commented Nov 10, 2022 • edited Loading

athei commented Nov 10, 2022

jasl commented Nov 10, 2022 • edited Loading

jasl commented Nov 11, 2022

jasl commented Nov 11, 2022

jasl commented Nov 11, 2022

jacogr commented Nov 11, 2022 • edited Loading

jasl commented Nov 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasl commented Nov 11, 2022

jacogr commented Nov 12, 2022

jasl commented Nov 12, 2022

jacogr commented Nov 12, 2022

jacogr left a comment

Choose a reason for hiding this comment

polkadot-js-bot commented Nov 15, 2022

jasl commented Nov 8, 2022 •

edited

Loading

jasl commented Nov 9, 2022 •

edited

Loading

statictype commented Nov 9, 2022 •

edited

Loading

jacogr left a comment •

edited

Loading

jacogr Nov 10, 2022 •

edited

Loading

jacogr commented Nov 10, 2022 •

edited

Loading

jacogr commented Nov 10, 2022 •

edited

Loading

jasl commented Nov 10, 2022 •

edited

Loading

jacogr commented Nov 11, 2022 •

edited

Loading

jacogr Nov 11, 2022 •

edited

Loading