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

chore: replacing all generic errors with FuelError #2549

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

msensys
Copy link
Contributor

@msensys msensys commented Jun 18, 2024

Closes #2001

@danielbate danielbate added the chore Issue is a chore label Jun 19, 2024
@danielbate
Copy link
Contributor

@msensys does this pick up all occurences?

@danielbate danielbate added this to the 0.x post-launch milestone Jun 19, 2024
Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

@msensys does this pick up all occurences?

It believe not - hence the "Relates to"?

@danielbate danielbate marked this pull request as draft June 19, 2024 08:58
@danielbate danielbate marked this pull request as ready for review June 19, 2024 08:58
@danielbate
Copy link
Contributor

Sorry @msensys was just trying to validate why the validate changeset CI stage hadn't run so converted to draft

@msensys
Copy link
Contributor Author

msensys commented Jun 19, 2024

@msensys does this pick up all occurences?

Maybe not. I will send some commits to pick up all occurrences.

Copy link
Contributor

@nedsalk nedsalk left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

To fully wrap this up, can you please go to the tests that are expecting these errors and use the expectToThrowFuelError utility?

For example, for this error, there exists this test. You'd change the test to look like this:

it('throws when setting configurable but predicate has none', async () => {
  await expectToThrowFuelError(
    () =>
      new Predicate({
        bytecode: predicateBytesTrue,
        abi: predicateAbiTrue,
        provider: wallet.provider,
        inputData: ['NADA'],
        configurableConstants: {
          constant: 'NADA',
        },
      }),
    new FuelError(
      ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
      'Predicate has no configurable constants to be set'
    )
  );
});

You can do a repo-wide search for the expected error message on each error you changed and there should be a corresponding test which you'd change as shown above. If a test doesn't exist for the error, you're welcome to add it.

@msensys
Copy link
Contributor Author

msensys commented Jun 19, 2024

Hey @nedsalk! The way that I found to resolve the problem of the test was this:

    it('throws when setting configurable but predicate has none', async () => {
      await expectToThrowFuelError(
        () =>
          new Predicate({
            bytecode: predicateBytesTrue,
            abi: predicateAbiTrue,
            provider: wallet.provider,
            inputData: ['NADA'],
            configurableConstants: {
              constant: 'NADA',
            },
          }),
        {
          code: ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
          message: expect.stringContaining('Predicate has no configurable constants to be set'),
        }
      );
    });

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

@msensys It looks like this PR will actually close this, correct?

If so, please change the description to Close #NNN instead of Relates to #NNN.

Also, if you add a comment on #2001, we can assign it to you.

@msensys
Copy link
Contributor Author

msensys commented Jun 19, 2024

@msensys It looks like this PR will actually close this, correct?

If so, please change the description to Close #NNN instead of Relates to #NNN.

Also, if you add a comment on #2001, we can assign it to you.

@arboleya, can I consider it as fix?

I associated this as patch due to what @petertonysmith94 said in its comment.

@arboleya
Copy link
Member

@msensys Oh, I assumed it would handle all generic errors, not only a subset.

Isn't that the case? If not, it won't be enough to close #2001.

@msensys
Copy link
Contributor Author

msensys commented Jun 19, 2024

@msensys Oh, I assumed it would handle all generic errors, not only a subset.

Isn't that the case? If not, it won't be enough to close #2001.

I'll cover them all, but I want to know about the name to proceed :D

@arboleya
Copy link
Member

@msensys What do you mean by "name"?

@msensys
Copy link
Contributor Author

msensys commented Jun 19, 2024

@msensys What do you mean by "name"?

Change the label of commit.

Btw, some tests use Error in the mock part. So changing this might not make sense.

@msensys msensys requested a review from nedsalk June 19, 2024 19:52
{
code: ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
message: expect.stringContaining('Predicate has no configurable constants to be set'),
}
Copy link
Contributor

@nedsalk nedsalk Jun 20, 2024

Choose a reason for hiding this comment

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

@msensys the expectToThrowFuelError utility internally checks for message (and any other property) equality. You shouldn't be passing this as its second argument:

{
  code: ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
  message: expect.stringContaining('Predicate has no configurable constants to be set'),
}

but rather this:

new FuelError(
  ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
  'Predicate has no configurable constants to be set'
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why, but the test isn't passing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nedsalk look at here:

- [FuelError: Predicate has no configurable constants to be set]
+ Object {
+   "VERSIONS": Object {
+     "FORC": "0.60.0",
+     "FUELS": "0.90.0",
+     "FUEL_CORE": "0.30.0",
+   },
+   "code": "invalid-configurable-constants",
+   "metadata": Object {},
+   "name": "FuelError",
+ }

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any failing tests; CI is green for all workflows.

But as @nedsalk said, you should not assert the error with an Object:

{
code: ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
message: expect.stringContaining('Predicate has no configurable constants to be set'),
}

Instead, you should use FuelError, as you did on the other occurrences:

new FuelError(
ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
`Error setting configurable constants: No configurable constant named 'NOPE' found in the Predicate.`
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I committed the change you mentioned. Take a look at CI

Copy link
Contributor

Choose a reason for hiding this comment

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

@msensys the test is failing because of this line catching and rethrowing the error with an updated message. To fix the failing test, change the expected message in your test to Error setting configurable constants: Predicate has no configurable constants to be set.; you were missing the Error setting configurable constants: part. We should maybe rethink this try/catch here, but it's out of the scope of this issue. Just changing the expected message is enough.

@arboleya arboleya changed the title chore: switching generic error to fuel error chore: replacing all generic errors with FuelError Jun 20, 2024
msensys and others added 2 commits June 20, 2024 17:48
@maschad
Copy link
Member

maschad commented Jun 20, 2024

@msensys I see that this PR still needs some work as there are quite a few places we still throw Error as opposed to FuelError (at least in 41 files). I'm going to move this PR to draft and you can re-open it to review once it's ready.

@maschad maschad marked this pull request as draft June 20, 2024 21:07
@arboleya
Copy link
Member

@msensys I see that this PR still needs some work as there are quite a few places we still throw Error as opposed to FuelError (at least in 41 files). I'm going to move this PR to draft and you can re-open it to review once it's ready.

Note that we should use the UNKNOWN code for unmapped errors.

However, to apply this, you'll need to wait until we merge this:

cc @maschad

@arboleya
Copy link
Member

arboleya commented Jun 21, 2024

Note that we should use the UNKNOWN code for unmapped errors.

Also, UNKNOWN errors must contain the rawError (see comment).

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

Successfully merging this pull request may close these issues.

Replace all occurrences of Error to FuelError
6 participants