Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for language level errors (LangError) #1450

Merged
merged 111 commits into from
Nov 16, 2022

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Oct 25, 2022

Implements the Message related changes of #1207.

The idea here is that every message now returns a Result<Message::Output, LangError>,
where a LangError is a type of error which doesn't originate from the contract itself,
nor from the underlying execution environment (so the Contracts pallet in this case).

An example of where this would arise is if a caller tries to use a non-existent message
selector for a contract. Previously, the contract would trap and not allow the caller to
do any sort of error handling if it encountered a non-existent selector.

This breaks the ABI in two ways: first, all contract messages now have a Result return
type, and second a new field, lang_err, will be introduced as part of the contract
spec. The second change allows other languages, such as Solang, to use an equivalent
LangError.

If you're curious, click here for a snippet of the new metadata for the Flipper contract.

"messages": [
  {
    "args": [],
    "docs": [
      " Flips the current value of the Flipper's boolean."
    ],
    "label": "flip",
    "mutates": true,
    "payable": false,
    "returnType": {
      "displayName": [
        "ink",
        "MessageResult"
      ],
      "type": 1
    },
    "selector": "0x633aa551"
  }],
"lang_error": {
  "displayName": [
    "ink",
    "LangError"
  ],
  "type": 3
},
{
  "id": 3,
  "type": {
    "def": {
      "variant": {
        "variants": [
          {
            "index": 1,
            "name": "CouldNotReadInput"
          }
        ]
      }
    },
    "path": [
      "ink_primitives",
      "LangError"
    ]
  }
}

TODO:

  • Add new LangError type to metadata
  • Add E2E test for demonstrating how to handle LangErrors
  • Make CI green

TODOs in Follow-Ups:

cc @xgreenx since you were the original poster for the issue

@HCastano HCastano requested a review from athei October 25, 2022 23:40
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach looks good, I think a unification of the LangError and DispathError is a good idea if you didn't already have that planned.

crates/ink/ir/src/ir/item_impl/message.rs Outdated Show resolved Hide resolved
crates/ink/ir/src/ir/item_impl/message.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/impls.rs Outdated Show resolved Hide resolved
crates/ink/src/reflect/dispatch.rs Outdated Show resolved Hide resolved
deploy-contract.sh Outdated Show resolved Hide resolved
examples/cross_chain_test/lib.rs Outdated Show resolved Hide resolved
crates/ink/codegen/src/generator/item_impls.rs Outdated Show resolved Hide resolved
crates/ink/codegen/src/generator/dispatch.rs Show resolved Hide resolved
crates/ink/codegen/src/generator/dispatch.rs Outdated Show resolved Hide resolved
@xermicus
Copy link
Contributor

xermicus commented Nov 4, 2022

@HCastano Can we use TypeSpec (together with an according builder method) in the contract spec instead, so it will work for Meta and Portable form? I was so free to push my proposed changes directly to your branch (also merged master and resolved the conflict), please have a look. If you are not comfortable with this just nuke my commits

Also CC @ascjones

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ the comprehensive e2e tests

#[ink(message)]
pub fn flip_check(&mut self) {
self.flipper
.flip_checked()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would testing err_flip_checked be useful do you think? As in it will be a Ok(Err(()))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be useful, but maybe in the context of a different set of tests around error handling

Copy link
Contributor

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding an error for NotPayable could be quite easy? Can be a follow up but calling return_value instead of using ? seems easy enough.

Apart from that it looks good to me. I compiled some simple contracts and the metadata looks sensible.

crates/ink/codegen/src/generator/dispatch.rs Outdated Show resolved Hide resolved
@HCastano HCastano changed the title Add ability to handle DispatchError Add support for language level errors (LangError) Nov 16, 2022
@HCastano
Copy link
Contributor Author

I think adding an error for NotPayable could be quite easy? Can be a follow up but calling return_value instead of using ? seems easy enough.

Yeah now that this is in place it should be almost trivial to add that. Figured it would be better left for a follow-up though

@xgreenx
Copy link
Collaborator

xgreenx commented Nov 21, 2022

I made a change to generate two sets of functions, the "original" unchecked function and the new "checked" functions (which are the ones with the Result<T, LangError> return types.

I think “unchecked” fits more for methods that return MessageResult, because we didn’t check the result it is why you can handle it=)

@athei
Copy link
Contributor

athei commented Nov 21, 2022

Most idiomatic would be using try_ as a prefix. This is what the standard library will use to allow panic free allocation soon. For example: Vec::try_push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change return type in metadata to allow common errors
5 participants