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

Map VM errors to FuelError error codes #2467

Closed
nedsalk opened this issue Jun 7, 2024 · 11 comments
Closed

Map VM errors to FuelError error codes #2467

nedsalk opened this issue Jun 7, 2024 · 11 comments
Labels
feat Issue is a feature

Comments

@nedsalk
Copy link
Contributor

nedsalk commented Jun 7, 2024

  • Create ErrorCode entry for every VM error
  • Every ErrorCode entry that maps to a VM error must have a test associated with it:
    • e.g. bring the VM to throw not enough coins to fit the target and:
      • Verify that a FuelError with the not-enough-coins code is thrown
      • Verify that FuelError.rawError corresponds to the serialized VM error - this would be done with a fetch spy

cc @LuizAsFight

@nedsalk nedsalk added the feat Issue is a feature label Jun 7, 2024
@petertonysmith94
Copy link
Contributor

Adding the cause for unknown errors might be easier?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause

@arboleya arboleya added this to the Caterpilar milestone Jun 9, 2024
@arboleya arboleya added p0 and removed offsite labels Jun 9, 2024
@LuizAsFight
Copy link
Contributor

hey @nedsalk thanks for the heads up

just for context check this file from wallet code

graphql api can return multiple errors, that's why we initially had only e.response?.errors , and after the FuelError was included we needed to add this conditional, forcing the creation of an array containing the error

do you think it makes sense to create 1 FuelError for each occurrence of errors that came from graphql api? that way returning always an array

@nedsalk
Copy link
Contributor Author

nedsalk commented Jun 12, 2024

I asked @FuelLabs/client the following question:

The graphql API returns an errors array when an error happens on the VM. In practice, are there cases where the VM will return more than one error? If there are none, could we make an assumption that there won't be any in the future? Having this assumption would improve ergonomics for users on our side - a thrown error would always be the error, not a wrapper for multiple errors in some cases and the error in others.

And @Dentosal responded:

My understanding is that the errors array is part of how GraphQL itself works. If you only request one VM operation, then there can only be one VM error.

@LuizAsFight based on this, we can extract the single error from the graphql errors array and throw it. We don't need to handle the multiple errors case because it never happens in reality. The wallet code that you linked to could then maybe be simplified because you wouldn't be working with arrays at all.

@arboleya arboleya modified the milestones: 1.0 caterpillar, 0.x post-launch Jun 12, 2024
@arboleya
Copy link
Member

I reduced the scope of this issue and moved a portion of it to the below issue, as it will be easier and provide immediate value even before we can map "all" errors.

Speaking of all errors, do we have a unified list of possible errors the VM can throw, or how else should we know when we mapped them all? I suspect this may be something we'll keep mapping as we go.

In this sense, I created a first issue here for a recurring error:

@nedsalk
Copy link
Contributor Author

nedsalk commented Jun 17, 2024

Speaking of all errors, do we have a unified list of possible errors the VM can throw, or how else should we know when we mapped them all? I suspect this may be something we'll keep mapping as we go.

We have a list of VM error codes in our repo already - it's used in a similar error throwing vein, and the full list can be found here.

@LuizAsFight
Copy link
Contributor

@nedsalk one incorrect query can return multiple errors

query {
  blocks (last: 10, first: "asd") {
    nodes {
      height
      asds
    }
  }
}

response

{
  "data": null,
  "errors": [
    {
      "message": "Invalid value for argument \"first\", expected type \"Int\"",
      "locations": [
        {
          "line": 2,
          "column": 20
        }
      ]
    },
    {
      "message": "Unknown field \"asds\" on type \"Block\".",
      "locations": [
        {
          "line": 5,
          "column": 7
        }
      ]
    }
  ]
}

@LuizAsFight
Copy link
Contributor

I remember to have seen multiple errors before in the wallet also in other scenarios than a wrong query

@nedsalk
Copy link
Contributor Author

nedsalk commented Jun 18, 2024

Those errors look like graphql-related errors thrown when writing custom queries. The SDK itself internally should never encounter them because we use Provider.operations, which is type safe because it's generated via graphql-codegen.

Given that custom queries are out of the SDK's scope (currently, depending on if we work on #1570), FuelError shouldn't be concerned with their failure modes; and even if they were in our scope, it's questionable if they should be integrated into FuelError or possibly be a different error class, e.g. GraphqlError, because they're not really domain-specific so that they're included into FuelError but rather they're related to the GraphQL protocol.

It would be great if you could verify if the other scenarios are graphql-related or not, though.

@LuizAsFight
Copy link
Contributor

LuizAsFight commented Jun 20, 2024

as the graphql natively expose errors array I would just safely keep the same pattern instead of hunt situations that multiple errors can happen. but that's my personal opinion as I like my code to be fault-proof. if multiple errors happen, I would be already prepared

if you wanna assume it will be always one error only, sounds good also no big deal, up to you guys

only downside I see is that if we figure this out later may need to refact something

@nedsalk nedsalk self-assigned this Jul 1, 2024
@nedsalk nedsalk removed their assignment Jul 11, 2024
@arboleya arboleya added p1 and removed p0 labels Jul 19, 2024
@arboleya arboleya removed this from the 0.x post-launch milestone Jul 19, 2024
@Torres-ssf
Copy link
Contributor

@nedsalk This seems to be a duplicate of #2208

@arboleya arboleya removed the p1 label Aug 2, 2024
@petertonysmith94 petertonysmith94 self-assigned this Aug 6, 2024
@arboleya arboleya added the temp:notion label Sep 8, 2024 — with Linear
@arboleya arboleya added the temp-linear label Sep 8, 2024 — with Linear
@petertonysmith94
Copy link
Contributor

Going to unassign myself from this for now.

@petertonysmith94 petertonysmith94 removed their assignment Sep 9, 2024
@danielbate danielbate closed this as not planned Won't fix, can't repro, duplicate, stale Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

No branches or pull requests

6 participants