Add more possibilities to call contract messages

[VargSupercolony]

The problem

The problem right now is that contract messages (txs & queries), which have a trait prefix, are only callable like myTraitMyMethod. Previously, the stringCamelCase function allowed commas, so we could differentiate between message name and trait name when calling the functions. Like this:

const myContractCallOutcome: ContractCallOutcome = await myContract.query['MyTrait,myMethod'](...)

But now, as the stringCamelCase function replaces the commas with whitespace (and camelCases the trait), it means we have only one way to call a contract function:

const myContractCallOutcome: ContractCallOutcome = await myContract.query.myTraitMyMethod(...)

As the trait names may grow longer, we end up calling simple methods like this:

const myContractCallOutcome: ContractCallOutcome = await myContract.query.myVeryLongTraitNameBalanceOf(...)

In some cases, we don't want the frontend calls to contain the trait names, and with this approach, it is almost impossible to remove the trait prefixes without doing hacks.

Proposed solution

The proposed solution is following:

1. For ink v3.0.0-rc6, as the trait names and method names are separated by a comma, leave a comma there, camelCase the method name and write to the query object. (Ends up like myContract.query['MyTrait,myMethod'])
2. For ink v3.0.0-rc7 and potentially later versions, as the trait names and methods are separated by ::, add the unchanged generated identifier to the query and tx objects. (Ends up like myContract.query['MyTrait::my_method'])
3. Regardless of the version, add the camelCased version of call as it is now. (Ends up like myContract.tx.myTraitMyMethod)

In the end, we want to end up with several ways to call the same ABI message.

I would be pleased to work on this issue personally if you need my help :)

[jacogr]

The API doesn't track or work on ink versions. So have no idea what versions you are referring to. What it does do is work on the version of the metadata, which is actually broader than ink itself, i.e. versioned metadata is also output by other compilers, for instance solang.

In both your examples, providing an ABI to see what the different outputs are is the best route.

[VargSupercolony]

Of course.

This is a metadata.json file generated for ink v3.0.0-rc6 (V1 metadata):

{
          "args": [],
          "docs": [
            " Returns the token name."
          ],
          "mutates": false,
          "name": [
            "PSP22Metadata",
            "token_name"
          ],
          "payable": false,
          "returnType": {
            "displayName": [
              "psp22metadata_external",
              "TokenNameOutput"
            ],
            "type": 19
          },
          "selector": "0x3d261bd4"
        }

This one is V3 metadata by ink v3.0.0-rc7:

{
          "args": [],
          "docs": [
            " Returns the total token supply."
          ],
          "label": "BaseErc20::total_supply",
          "mutates": false,
          "payable": false,
          "returnType": {
            "displayName": [
              "Balance"
            ],
            "type": 0
          },
          "selector": "0x8244a1ad"
        }

So in the V1 metadata, the 'name' key denotes the method name, and it's an array of trait prefix + method name. In V3, it's located in the 'label' key, and follows Rust syntax of Trait::method. My proposal is to include multiple ways to call method, like myTraitMyMethod & ['MyTrait::my_method'] (for V3) or ['MyTrait,myMethod'] (for V1).

[jacogr]

So I understand why in V1 the name came through with ,, it was wrongly serialized.

As for the V3 with the :: labels, also have not seen this one. Both full .contract files (or similar ones) would be great to add as tests - neither form appears anywhere in the V0-V3 samples that are available.

For the first - checks against the v1ToLatest function to ensure it ends up in the same form as the current V3 traits

For the second - seeing what it actually does in practice and then being able to formulate some way around it.

[VargSupercolony]

I can do that, just assign me to this issue

[VargSupercolony]

So for the first, I checked how the messages containing traits are parsed - and indeed they are in a different format.
This is how the messages get parsed from V3 metadata:

And V1 metadata:

[jacogr]

Indeed, that is what #4471 addresses - it changes the up-conversion to combine the name -> labels in the same way as natively supplied in V2+, aka with ::

This is where more actual real-world contracts are needed, none of the versions tested against has the same type of labelling, https://github.com/polkadot-js/api/tree/master/packages/api-contract/src/test/contracts

[VargSupercolony]

Ok, got it. Should I provide example contracts & tests for conversions?

[jacogr]

That would be great, one V1 with the namespaces (https://github.com/polkadot-js/api/tree/master/packages/api-contract/src/test/contracts/ink/v1) and a V2/V3 (doesn't matter, they would both be equivalent with label) would help a lot.

[VargSupercolony]

I'm on it 👌

[VargSupercolony]

#4472

[VargSupercolony]

Pining the issue to continue the discussion regarding method calls.
Since we completed the first thing we wanted to do, let's get back to the original point of the proposal:

This is what the contract query object looks like for me on my front-end (metadata V1, @polkadot/api v ^7.4.1). As you can see, there is only one way to call the methods of the contract, which is by camelCasedTraitNameAndMethodName. I want to suggest adding two more ways of calling the contracts' methods (for all metadata versions):

• Call using contract.query/tx['PSP22.balanceOf'] or contract.query/tx.PSP22.balanceOf syntax
  This would actually create a really neat way of grouping the messages by traits, so PSP22 object would contain accordingly namespace messages.

• Call using contract.query/tx['PSP22::balance_of'] syntax

As the new generated metadata with the label attribute suggests us using Rust's syntax, I feel like it would be great to add this to the query/transaction objects. (As well as display it this way on the contracts UI, but that's a different issue 😄 )

Again, feel free to assign me to these tasks.
Looking forward to hearing your thoughts on the suggestion

[jacogr]

Indeed, now that the discrepancies have been addressed, back to the original.

The contracts contract.{tx, query}.<method> actually has a flaw compared to the methods on the api.{tx, query}.* interfaces. The API variants also exposes the actual metadata, e.g. api.query.system.account.meta in the ABI's these are not available. (Just logged that here #4484).

Basically with the metadata available, things like UIs can then display additional extra information, i.e. original trait info. (It can be gotten to at this point, but it really does need to be consistent, and here it is not yet)

So generally the API itself never decorates using Rust syntax, it decorates with JS syntax - which we everywhere try to get to via stringCamelCase. (Contracts, API, etc)

So on the options you laid out -

• I'm not in favor of exposing contract.{tx, query}['PSP22::balance_of'] at all - this basically means that we have multiple names for the same underlying method. We have a 1-to-1 mapping between the interfaces and what is on the ABIs
• The contract.{tx, query}.<nameSpace>.<method> could be interesting. Once again, this would make users have to jump through some hoops, e.g. contract.{tx, query}.something may have something as a namespace or method, so it does break downstream code and removes consistency.

Something like contract.ns.psp22.{query, tx}.balanceOf may be worth investigating. (Or something similar to the format, e.g. contract.ns.query.p2p22.balanceOf). However once again, here we are in JS camelCase syntax, not Rust syntax.

[VargSupercolony]

• I'm not in favor of exposing contract.{tx, query}['PSP22::balance_of'] at all - this basically means that we have multiple names for the same underlying method. We have a 1-to-1 mapping between the interfaces and what is on the ABIs

I would say that the interfaces now make calling methods harder :) For example, from the first look and without some debugging, I would have no idea how to call methods, especially when the namespaces and method names are camelCased and concatenated. This approach is favorable cause we directly use the label attribute, just like in the ABI.

• The contract.{tx, query}.<nameSpace>.<method> could be interesting. Once again, this would make users have to jump through some hoops, e.g. contract.{tx, query}.something may have something as a namespace or method, so it does break downstream code and removes consistency.

Well, after the comma change in stringCamelCase somewhere in November-December 2021, all of the calls in the frontend have broken :) So changes like this will probably break some code for the users. But having syntax like this, we can really cut down on the internal information the frontend gets of the contracts. Consider this code:

const myContract: Contract = await new ContractPromise(api, abi, address);
for (const prop in myContract) {
  if (typeof(prop) === 'object') { // forgive me my js/ts knowledge :)
    for (const method in prop) {
      myContract[method] = myContract[prop][method];
    }
    delete myContract[prop];
  }
}

This would copy out all of the methods from all of the namespaces that we know of, while in theory leaving non-namespaced methods unchanged, hence providing less information to the front-end about the contract structure (e.g., when storing the contract in the window).

To sum up, I would really want to have the second syntax (or some form of it) available.

[jacogr]

I can only verify what I can see - since I had no tests for namespaced methods (never actually seen them) and nobody complained, well, it is what it is :)

I understand you would like Rust-like syntax, however we will not provide it - the API is following JS-like way of doing things, not transferring Rust-isms to JS. The same approach is followed through the whole API in all naming.

The same applies to consistency, i.e. contract.{tx, query}.<something> need to be of a specific type. Not one-of and certainly not without any major release, we don't break userspace code without a very good reason. So the only namespaced syntax that would fit in these constraints is the 3rd option, with no guessing anywhere.

[VargSupercolony]

I understand you would like Rust-like syntax, however we will not provide it - the API is following JS-like way of doing things, not transferring Rust-isms to JS. The same approach is followed through the whole API in all naming.

I don't agree that it's Rust-like syntax. Accessing a property of an object via ['propName'], regardless of the property name, is JS-like way. Again, the generated label attribute adds to it. Moreover, I suspect it's the way the method selector is generated for api.contract.tx/query objects and in the ABI (taking a label, hashing, and taking a fixed number of bytes). I see this being the common ground for contracts written in Rust and JS.

Still, if you don't want to have Rust-like property names, the 3rd option is fine. But since some methods can be non-namespaced, they'd have to be accessed without .ns (contract.query.<something>).

Aaand if we decide to keep the things as they are - it's the worst solution, as namespace methods might start with reallyLongTraitPrefixes. Throw in Upgradable - you'd get reallyLongTraitPrefixesUpgradableAndFinallyTheMethodName :)

[jacogr]

The last example is 100% fine, it is exactly the same way the metadata v14 types names are generated.

What we are trying to do is to ensure users never have to resort to ['propName'], so we can always use .proName with no string-like identifiers anywhere (including in generated interfaces, e.g. still on the list for contracts in #3337).

[VargSupercolony]

The last example is 100% fine, it is exactly the same way the metadata v14 types names are generated.

Sorry, missed it, what example? :)

[jacogr]

reallyLongTraitPrefixesUpgradableAndFinallyTheMethodName see for instance https://github.com/polkadot-js/api/blob/master/packages/types-augment/src/lookup/kusama.ts#L201 (the type names do have some cleanup logic, e.g. removing ::Trait:: and some others along the way, but for the most part just converts the paths)

[xgreenx]

Hi, I also want to participate in that discussion=)

Aaand if we decide to keep the things as they are - it's the worst solution, as namespace methods might start with reallyLongTraitPrefixes. Throw in Upgradable - you'd get reallyLongTraitPrefixesUpgradableAndFinallyTheMethodName :)

I agree with Varg regarding the unreadable long name of the functions. We need to find a way how we can improve that.

What we are trying to do is to ensure users never have to resort to ['propName'], so we can always use .proName with no string-like identifiers anywhere (including in generated interfaces, e.g. still on the list for contracts in #3337).

Also, I agree with Jacogr, that we should resolve all questions to ensure users never have to resort ['propName']. I think that ["PSP22::balance_of"] is handled by #4484 because the developer always can get that information from the metadata(from label).

Call using contract.query/tx['PSP22.balanceOf'] or contract.query/tx.PSP22.balanceOf syntax
This would actually create a really neat way of grouping the messages by traits, so PSP22 object would contain accordingly namespace messages.

Something like contract.ns.psp22.{query, tx}.balanceOf may be worth investigating. (Or something similar to the format, e.g. contract.ns.query.p2p22.balanceOf). However once again, here we are in JS camelCase syntax, not Rust syntax.

I think the idea with contract.{ query, tx }.<interface>.<method> covers everything very well.

• It keeps the name of the functions readable.
• It still is JS syntax.

Also, I think that it is better than contract.ns.<interface>.{query, tx}.<method> because we keep consistent of the API during the case with the interface and without.

So if method without interface:
contract.{ query, tx }.balanceOf
If with interface:
contract.{ query, tx }.PSP22.balanceOf

I think we don't need to camelcase the name of the interface and take it as it was in the label.

The same idea we can use in UI, to improve readability. Near the method balanceOf, we can write the name of the interface.

[jacogr]

Needs to be verified/test cases added, but this does expand into namespaces under contract.ns.* - #4487

(It is actually really unfriendly atm without TS augmentation on a per-contract basis)

As per the rest of the API, everything is camelCase. I'll re-iterate, not going to mix interfaces and methods under a single point of the existing interface, it is a nightmare. The fact that it may be one or an other is a major PITA everywhere, in the API contracts code and the use thereof. (And I certainly won't introduce a breaking change like that lightly, major semver or not)

... having said all that, if something like .ns. proves very useful, can certainly consider making it the default in the future. Despite my reservation above. (Especially if proper TS interfaces can be generated on a per-contract basis - it will remove the "this is a major PITA" from end-users)

[xgreenx]

As per the rest of the API, everything is camelCase. I'll re-iterate, not going to mix interfaces and methods under a single point of the existing interface, it is a nightmare. The fact that it may be one or an other is a major PITA everywhere, in the API contracts code and the use thereof. (And I certainly won't introduce a breaking change like that lightly, major semver or not)

Why it will be a nightmare? Because the user doesn't know is that method or an interface? For that case, we can introduce a type for each field in query and tx.

assert(contract.query.balanceOf.type == `method`);
assert(contract.tx.balanceOf.type == `method`);
assert(contract.query.PSP22.balanceOf.type == `interface`);
assert(contract.tx.PSP22.balanceOf.type == `interface `);

... having said all that, if something like .ns. proves very useful, can certainly consider making it the default in the future. Despite my reservation above. (Especially if proper TS interfaces can be generated on a per-contract basis - it will remove the "this is a major PITA" from end-users)

Do you mean auto-generated code from the ABI, like classes and interfaces? I think we still can generate everything clearly for the developer:

class Query {
  PSP22: PSP22Interface;
 
  balanceOf() {
    ...
  }
}

interface PSP22Interface {
  balanceOf() {
    ...
  }
}

BTW, what does "PITA" mean?=D

[jacogr]

PITA = Pain In The Ass. Sprinkling in asserts everywhere is exactly that - and inconsistent with the API.

The above is not a negotiation- sadly I personally have to deal with breakages and the support around it.

[xgreenx]

PITA = Pain In The Ass. Sprinkling in asserts everywhere is exactly that - and inconsistent with the API.

I didn't mean to add asserts=) I meant that if for the developer is important to know that the field is not a method and it is an interface, that information can be provided via the type field.

The above is not a negotiation- sadly I personally have to deal with breakages and the support around it.

Okay, maybe you can provide an example, I'm only want to understand what kind of problems that can cause.

I checked the PR:

• contract.ns.{ query, tx }.<interface>.<method> seems much better than contract.ns.<interface>.{query, tx}.<method> so it can be an option=) But we still introduce a new ns field=(
• Hmm, seems that in the case of empty path Namespaced<ContractQuery<ApiType>> and expandNs will generate MapMessageQuery<ApiType>. So, are you sure that it will cause PITA, maybe contract.query can be Namespaced<ContractQuery<ApiType>>?
• Do you plan to camelCase in the name of the path? Will it be contract.ns.{ query, tx }.PSP22.<method> or contract.ns.{ query, tx }.psp22.<method>?(I prefer the first variant)

[jacogr]

It is the second variant. The API always applies camelCase.

As I stated before - the interface or method may become standard somewhere along the way, but at this point not breaking userspace for the 1% of cases where these are used. And certainly not bumping to a major version to introduce this breakage anytime soon.

TL;DR on this one, not budging from meeting halfway, since the fallout is on my shoulders alone. When and if -

• TS interfaces are a reality
• namespace usage become more mainstream

… then, can re-evaluate

[xgreenx]

Okay, so when the usage of the namespaces/interfaces will be popular in ink!, we can elaborate on merging ns into query and tx?=)

It is the second variant. The API always applies camelCase.

Maybe is possible to keep the name of the namespace/interface as it is without camelCase?
Because autogenerated .psp22 and .wrappedPSP22 looks worse with comparison to original naming .PSP22 and .WrappedPSP22

And it also changes the behavior of namespaces. For example psp22 and PSP22 are different namespaces in the code of the contract. It causes different selectors for methods. And if the developer decided to use both in one contract, JS will parse that incorrectly

The same idea we can use in UI, to improve readability. Near the method balanceOf, we can write the name of the interface.

And what do you think about the idea with UI to show methods without namespace prefix and show the info about namespace in a separate place?

[jacogr]

This is not the place for UI, sadly the queue there is different - although I do both, I cannot track in both places - information overload.

I believe I answered the rest at least twice now :)

[xgreenx]

Okay, so when the usage of the namespaces/interfaces will be popular in ink!, we can elaborate on merging ns into query and tx?=)

For that question I need "yes" or "no", to be sure that I understood you correctly =)

Regarding namespace and camelCase, I only want to highlight, that the code of the contract is sensitive to registers of letters in the namespaces and it can cause an error during the parsing of that contract(when the developer has in the contract both psp22::balance_of and PSP22::balance_of methods). If you are okay with that, then I tried to change your mind about that=D