Skip to content

feat: propagate data.cause as cause in JsonRpcError constructor - #140

Merged
legobeat merged 5 commits into
MetaMask:mainfrom
legobeat:propagate-data-cause
May 31, 2024
Merged

feat: propagate data.cause as cause in JsonRpcError constructor#140
legobeat merged 5 commits into
MetaMask:mainfrom
legobeat:propagate-data-cause

Conversation

@legobeat

@legobeatlegobeat commented May 12, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

This maps the tc39 error cause field on JsonRpcError instances to data.cause, if it exists and is an object. No further validation is performed.

This is done because adding es2022 or dom to tsconfig lib are not being considered at this point.

Related

Alternative

@legobeat
legobeatforce-pushed the propagate-data-cause branch from 88f1100 to d4f192dCompareMay 12, 2024 23:56
@legobeat
legobeatforce-pushed the propagate-data-cause branch from d4f192d to e6350afCompareMay 13, 2024 00:05
@legobeatlegobeat mentioned this pull request May 13, 2024
Closed
@legobeat
legobeat marked this pull request as ready for review May 29, 2024 02:00
@legobeat
legobeat requested a review from a teamMay 29, 2024 02:00
rekmarks
rekmarks reviewed May 30, 2024
View reviewed changes
Comment threadtsconfig.json Outdated
@legobeat
legobeat requested a review from rekmarksMay 31, 2024 00:12
@legobeat
legobeatforce-pushed the propagate-data-cause branch from 74a6837 to 4b265fdCompareMay 31, 2024 01:26
@legobeat
improve cause-handling
09d433a
- break out util function dataHasCause with test
- comment clarity
- use native causes when available
note: jest coverage depends on runtime
@legobeat
legobeatforce-pushed the propagate-data-cause branch from 4b265fd to 09d433aCompareMay 31, 2024 01:26
@legobeat

legobeat commented May 31, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Validated locally that tests also pass on Node.js v14, which definitely doesn't have support for causes. We also see the expected increase in jest coverage, there.

@legobeat
legobeat requested a review from a teamMay 31, 2024 01:44
@legobeat

legobeat commented May 31, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Updated with some improvements lifted over from #141 :

  • Try to set cause natively via superclass constructor; fall back to setting manually
    • Use safer methods Object.prototype.hasOwnProperty.call and Object.assign in fallback
  • Comment clarity after feedback
  • Minor type refactor for consistency

This branch has been published for evaluation as @metamask/rpc-errors@6.3.0-rc1
Preview in extension

legobeat
legobeat commented May 31, 2024
View reviewed changes
Comment threadjest.config.js
functions: 94.44,
lines: 92.85,
statements: 92.85,
branches: 91.89,

@legobeatlegobeatMay 31, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

#141 (comment)

legobeat added a commit to legobeat/metamask-extension that referenced this pull request May 31, 2024
@legobeat
update @metamask/rpc-errors
64cc736
published from 08108c535cf5ee25603715cbfae0cd9aa3f2dea6
MetaMask/rpc-errors#140
@legobeatlegobeat mentioned this pull request May 31, 2024
Merged
10 tasks
rekmarks
rekmarks reviewed May 31, 2024
View reviewed changes
Comment threadsrc/classes.ts Outdated
rekmarks
rekmarks approved these changes May 31, 2024
View reviewed changes

@rekmarksrekmarks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@legobeat
legobeat merged commit 1b44940 into MetaMask:mainMay 31, 2024
@legobeat

legobeat commented May 31, 2024•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

If any reviewers are still looking at this: Pending review in #144 so follow-ups are still NBD, if you see any room for improvement.

@legobeatlegobeat mentioned this pull request May 31, 2024
Closed
@legobeatlegobeat mentioned this pull request Jun 11, 2024
Closed
12 tasks
Gudahtt pushed a commit to MetaMask/metamask-extension that referenced this pull request Oct 16, 2024
@legobeat
fix(deps): update from eth-rpc-errors to @metamask/rpc-errors (cause …
70e2c08
…edition) (#24496)
## **Description**
- Upgrade from obsolete `eth-rpc-errors` to `@metamask/rpc-errors`
- This introduce handling of error causes See [here](MetaMask/rpc-errors#140) for some
context.
[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24496?quickstart=1)
## **Related issues**
- #22871
#### Blocked by
- [x] MetaMask/rpc-errors#158
- [x] MetaMask/rpc-errors#144
- [x] MetaMask/rpc-errors#140
#### Blocking
- #22875
## **Manual testing steps**
## **Screenshots/Recordings**
### **Before**
### **After**
## **Pre-merge author checklist**
- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
## **Pre-merge reviewer checklist**
- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
Gudahtt pushed a commit to MetaMask/metamask-extension that referenced this pull request Oct 21, 2024
@legobeat@Gudahtt
fix(deps): update from eth-rpc-errors to @metamask/rpc-errors (cause …
b4c1f9c
…edition) (#24496)
- Upgrade from obsolete `eth-rpc-errors` to `@metamask/rpc-errors`
- This introduce handling of error causes
See [here](MetaMask/rpc-errors#140) for some
context.
[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24496?quickstart=1)
- #22871
- [x] MetaMask/rpc-errors#158
- [x] MetaMask/rpc-errors#144
- [x] MetaMask/rpc-errors#140
- #22875
- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rekmarksrekmarksrekmarks approved these changes
@FrederikBoldingFrederikBoldingAwaiting requested review from FrederikBolding
@MrtenzMrtenzAwaiting requested review from Mrtenz
@mcmiremcmireAwaiting requested review from mcmire
@MajorLiftMajorLiftAwaiting requested review from MajorLift
@naugturnaugturAwaiting requested review from naugtur
@kumaviskumavisAwaiting requested review from kumavis
@GudahttGudahttAwaiting requested review from Gudahtt

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@legobeat@rekmarks