Cross contract passes wrong `AccountId` to `seal_call` on substrate

[athei]

Please refer to this posting for further information: https://substrate.stackexchange.com/questions/202/calling-the-function-of-an-other-contract-in-solidity

I posted an answer there with the results of my debugging results: Solang somehow passes the wrong AccoundId to seal_call. I verified that by debugging output in the node.

[extraymond]

I think it might possible pallet-contract now takes a salt argument. And the integration test uses old pallet-contract code, which didn't have that as a option, also the polkadot-js api used in integration tests didn't take that salt argument either.

This is my related substrate so issue. https://substrate.stackexchange.com/questions/714/cross-contract-call-failure-when-using-different-salt-to-instantiate-contract

As you can see, the address generated with different salt argument ends up having different behavior. This is the pr on substrate that introduced the usage of salt in account generation https://github.com/paritytech/substrate/pull/7482

[extraymond]

Some update on playing with the test case of issue666, I've tried different ways to generate salt and seems the one used by polkadot.js might be the culprit, I tried the following with subxt, as a comparison with directly using polkadot.js app's contract tab

1. salt = account::nonce -> passed
2. same method before the above pr, salt = Runtime::Hashing::hash(input_data.encode()) -> worked, although account will collide like mentioned in the above pr.
3. submit empty vec![] as nothing -> passed
4. mimic polkadot.js/app, salt = random([u8; 32]) -> passed

Below is the related salt in rust

let salt = api.client.fetch_nonce::<DefaultAccountData>(signer.account_id())
                   .await?.encode(); // salt as nonce.encode()

let salt = <RuntimeConfig as subxt::Config>::Hashing::hash(&selector).encode(); // salt = Hash(input_bytes)

let salt = vec![];

let salt = rand::random::<[u8; 32]>().to_vec(); // salt = random u8_array

All the above cases was tested with the same tx's mortality as one generated when using polkadot.js contracts tab. Polkadot.js is using randomAsHex method to generated salt, and the method is calling this: https://github.com/polkadot-js/common/blob/0fa848420841909a3cc9938501a84cab02d48a27/packages/util-crypto/src/random/asU8a.ts#L22-L24

[extraymond]

I've added a test repo against substrate-contract-node. https://github.com/extraymond/substrate-contracts-node/tree/solang-test-0.12.0/runtime

All four types of salt value in the cross-contract calls test case can be executed without issues, which is really weird considering doing this from the UI will always show ContractTrapped. Hopefully this is a bug from the front-end.

@athei @seanyoung If you have the free time I would like to invite you to look at my test repo and see where I can help. Thanks in advance.

[extraymond]

Some updates on my tests: For comparison, I've added ink/solidity cross contract tests, and the good news is that all 4 directions works regardless of how the salt is generated. Directions being sol-call-ink, ink-call-sol and sol-to-sol and ink-to-ink.

I do observed some mismatches around the schemas between ink contracts and the one solang generated:

1. ink contracts group their abi under path of V3.spec field where solang is just .spec
2. some keywords are different, V3.spec have returnType where solang generates return_type
3. message name is now label for V3.spec
4. V3's types field is structures of {id: $id, type: {def, path}} where solang have {def}

I believe it's not very far from being 100% compatible against pallet-contracts v3 once we can make the generated contract file respect one of the supported version of metadata.

https://github.com/extraymond/substrate-contracts-node/blob/17d4bf7e1043f2310304861d8a85f7d37cbc00e7/runtime/src/tests/contracts.rs#L467-L549

[seanyoung]

@extraymond that is very interesting. Either solang never generated the correct metadata in the first place, or the metadata has changed somewhat.

Would be possible to amend the metadata generation in solang? The contract-metadata crate in solang also needs updating to 1.0, see https://github.com/hyperledger-labs/solang/blob/main/Cargo.toml#L30

Let me know if I can help in any way.

[extraymond]

@seanyoung

Thanks! I'm currently comparing solang's metadata generation behavior with cargo-contract ATM. What I found right now different between solang's metadata and cargo-metadata is in the spec field.

It appears to me that the internal of spec is somewhat tight to the development of ink. When cargo-contract generate the spec field, it import ink-metadata and assumes the project was built for ink. e.g

Where solang wasn't depending the internal of ink-metadata, from this point two project differs and hence the target output wasn't sharing the same primitives and all that.

I'm thinking potentially three ways to tackle this:

1. Temporary solution, either via external method or add a final stage in solang to convert the spec field to conform to ink's. Since some of the types in ink_metadata is private.
2. Forking ink_metadata so that other team can take the primitives and have the same behavior to generate the schema without having the assumption that this is a ink project.
3. If it doesn't make sense to use ink-metadata, for example hard to provide the same behavior of type registry, maybe a change in client-side should be made, so that js side can allow non-ink project to inject method to encode/decode the abi.

[seanyoung]

@seanyoung

Thanks! I'm currently comparing solang's metadata generation behavior with cargo-contract ATM. What I found right now different between solang's metadata and cargo-metadata is in the spec field.

It appears to me that the internal of spec is somewhat tight to the development of ink. When cargo-contract generate the spec field, it import ink-metadata and assumes the project was built for ink. e.g

Yes, I would agree with that. Right now I think ink! and solang are the only producers of the metadata format. This may change in future as more languages are added.

Where solang wasn't depending the internal of ink-metadata, from this point two project differs and hence the target output wasn't sharing the same primitives and all that.

I'm thinking potentially three ways to tackle this:

1. Temporary solution, either via external method or add a final stage in solang to convert the spec field to conform to ink's. Since some of the types in `ink_metadata` is private.

Last time I looked, ink_metadata was not usable for this, so I hand-coded the metadata format in solang. There is some shared code in the contract-metadata crate but that does not include the types.

2. Forking `ink_metadata` so that other team can take the primitives and have the same behavior to generate the schema without having the assumption that this is a `ink` project.

3. If it doesn't make sense to use ink-`metadata`, for example hard to provide the same behavior of type registry, maybe a change in client-side should be made, so that js side can allow non-ink project to inject method to encode/decode the abi.

I'm not sure what the right solution is here. However, the code in solang that produces the metadata for types is not huge (a few hunderd lines, everything called from ty_to_abi.

Forking ink_metadata is likely more work than adjusting these lines.

[seanyoung]

I had another idea. Rather than forking/updating ink_metadata, how about moving some structures into contract_metadata for generating the type metadata, for example the registry. This could then be re-used by both ink_metadata and solang (pending agreement/design of course).

[athei]

I think the ink! team would be happy to restructure the crates so that they can be used by other projects. I am just not an expert on metadata. Maybe you can open an issue there and state what would be necessary from your site so that you can use it as-is?

[seanyoung]

I think the ink! team would be happy to restructure the crates so that they can be used by other projects. I am just not an expert on metadata. Maybe you can open an issue there and state what would be necessary from your site so that you can use it as-is?

Great! This needs some design so that could be a great place to start that discussion.

[extraymond]

@athei @seanyoung

Taking from the discussion above, can I summarize it into actions needed below?

1. ink: extract ink-metadata out of the metadata generation process of cargo-contract and expose inner types to public
2. solang: replace bits in the generation of spec using the extracted crate above

[seanyoung]

@athei @seanyoung

Taking from the discussion above, can I summarize it into actions needed below?

1. `ink`: extract `ink-metadata` out of the metadata generation process of `cargo-contract` and expose inner types to public

2. `solang`: replace bits in the generation of `spec` using the extracted crate above

I think what @athei is suggesting is to open an issue on the ink! repo and start a discussion there.

[extraymond]

Got it, let me continue on there and sync back once something clicks.

[seanyoung]

Got it, let me continue on there and sync back once something clicks.

Please put @seanyoung in the issue or link it here, thank you!

[xermicus]

Fixed a while ago