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

fix: Return error as sent from the UI instead of throwing a new one #4610

Merged
merged 6 commits into from
Aug 15, 2024

Conversation

pnarayanaswamy
Copy link
Contributor

@pnarayanaswamy pnarayanaswamy commented Aug 14, 2024

Explanation

When signatures are rejected the UI sends some extra information in the error data property. This information is stripped off when the error is re-thrown in the controller. This change aims as throwing the error back to the UI as received.

Removes the @metamask/rpc-errors package from the Signature controller as this is not used anymore.

References

Changelog

None

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@@ -521,10 +519,10 @@ describe('SignatureController', () => {
it('throws if approval rejected', async () => {
messengerMock.call
.mockResolvedValueOnce({}) // LoggerController:add
.mockRejectedValueOnce({}); // ApprovalController:addRequest
.mockRejectedValueOnce({ message: 'User rejected the request.' }); // ApprovalController:addRequest
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are mocking the request, how do we ensure 'User rejected the request.' is still being thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only ensure whatever error was thrown by the client is returned as is. It is the responsibility of the client/UI in this case to throw the right error.

@@ -432,7 +431,7 @@ export class SignatureController extends BaseController<
);

resultCallbacks = acceptResult.resultCallbacks;
} catch {
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

Very minor, but I think we try and avoid abbreviation so this could also be error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -533,7 +531,6 @@ describe('SignatureController', () => {
{ parseJsonData: true },
),
);
expect(error instanceof EthereumProviderError).toBe(true);
expect(error.message).toBe('User rejected the request.');
Copy link
Member

Choose a reason for hiding this comment

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

For extra stability, is there value in including a data property in the thrown error also, to ensure that too is thrown unaltered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -40,7 +40,6 @@ import {
PersonalMessageManager,
TypedMessageManager,
} from '@metamask/message-manager';
import { providerErrors } from '@metamask/rpc-errors';
Copy link
Member

Choose a reason for hiding this comment

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

Is there anymore references to this package or can we remove @metamask/rpc-errors from package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@pnarayanaswamy pnarayanaswamy requested a review from a team as a code owner August 15, 2024 08:32
@pnarayanaswamy pnarayanaswamy merged commit d1202fd into main Aug 15, 2024
116 checks passed
@pnarayanaswamy pnarayanaswamy deleted the confirmation-alert-metrics branch August 15, 2024 15:11
@pnarayanaswamy
Copy link
Contributor Author

@metamaskbot publish-preview

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

Successfully merging this pull request may close these issues.

3 participants