Skip to content

Refactor RPC Methods logic (Fix network change)#3341

Merged
andrepimenta merged 15 commits into
mainfrom
fix/RPCMethods-refactor
Jan 27, 2022
Merged

Refactor RPC Methods logic (Fix network change)#3341
andrepimenta merged 15 commits into
mainfrom
fix/RPCMethods-refactor

Conversation

@andrepimenta

@andrepimenta andrepimenta commented Oct 26, 2021

Copy link
Copy Markdown
Member

Description

  • Moves the RPC Methods logic from the browser tab to its own file.
  • No more dependency on the Browser Tab props.
  • No more need for reloading after changing networks.

@cortisiko How to test:

@andrepimenta andrepimenta requested a review from a team as a code owner October 26, 2021 12:17
@andrepimenta andrepimenta added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 26, 2021
@andrepimenta andrepimenta changed the title Refactor RPC Methods logic Refactor RPC Methods logic (Fix network change) Oct 26, 2021
sethkfman
sethkfman reviewed Oct 26, 2021
View reviewed changes
Comment thread app/core/RPCMethods/RPCMethodMiddleware.js
@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 27, 2021
@aostrun aostrun mentioned this pull request Oct 28, 2021
Closed
3 tasks
@Cal-L Cal-L added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Oct 28, 2021
cortisiko
cortisiko requested changes Nov 2, 2021
View reviewed changes

@cortisiko cortisiko 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.

found a couple issues:

Issue 1
My eth balance does not update correctly when I switch networks.

For example:

I have eth in my wallet on main net and on ropsten. If I were to connect to uniswap dapp while on main net my eth balance looks correct.

If I were to switch my network from main net to ropsten my eth balance does not reflect what I currently have in my wallet on ropsten. Instead my balance reflects my main net balance.

See here
Notice this does not happen in prod on v3.4.1

To reproduce:
With a wallet that has eth on both main net and ropsten open the browser and go to app.uniswap.org
Connect your wallet.
Pay attention to your current ETH. Switch networks to ropsten
Wait 3-4 seconds. Notice your wallet balance does not update correctly.

Issue 2

I get a couple errors when I connect to a dapp:

(A)
ERROR: header not found.
And
(B)
{"originalError": [Object]}, "message": "undefined is not an object (evaluating '_.default.wallet_switchEthereumChain')"}

To reproduce:
With a wallet that has eth on both main net and ropsten open the browser and go to app.uniswap.org notice (A)
Connect your wallet.
Switch networks from main net to ropsten
Notice (B)

AND

Go to the test dapp: https://metamask.github.io/test-dapp/
And try tapping on “ADD XDAI CHAIN”
Notice (B)

@cortisiko cortisiko added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Nov 2, 2021
@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Nov 15, 2021
@cortisiko

cortisiko commented Nov 24, 2021

Copy link
Copy Markdown
Member

@andrepimenta this ticket is mainly good but I encountered a couple of errors while testing. Not sure if they are anything to worry about so feel free to dismiss them.

It is not the end of the world but time to time when I attempt to sign eth on the test dapp: https://metamask.github.io/test-dapp/

I get: [MetaMask DEBUG]: [Error: undefined is not an object (evaluating '_messageParams$meta.title')]

It is not really consistent enough for me to record it.

To reproduce:
Go here: https://metamask.github.io/test-dapp/
Connect your wallet
Scroll to the bottom of the page and tap on “sign ETH”
The error should appear

this guy is still happening: Error: [ethjs-rpc] rpc error with payload {"id":968630319409,"jsonrpc":"2.0","params":[{"to":"0x","data":"0x691f3431c7db9ae18f13a98c6f23dbad809949c7b232f627f38d8e0b0cf809d65f1cb206"},"latest"],"method":"eth_call"}

See the first instance of when this was reported here in slack.

To reproduce:

switch from a custom network( avalanche or polygon) to main net and vice versa.

@cortisiko cortisiko added QA'd but questions A QA run through has been done but you need clarification on minor issues you found and removed QA in Progress QA has started on the feature. labels Nov 24, 2021
rickycodes
rickycodes reviewed Dec 6, 2021
View reviewed changes
Comment thread app/core/RPCMethods/RPCMethodMiddleware.ts Outdated
@andrepimenta andrepimenta added the Priority - High Task with high priority label Jan 10, 2022
@andrepimenta andrepimenta added QA Passed QA testing has been completed and passed and removed QA'd but questions A QA run through has been done but you need clarification on minor issues you found labels Jan 18, 2022
@andrepimenta andrepimenta mentioned this pull request Jan 18, 2022
Closed
3 tasks
sethkfman
sethkfman approved these changes Jan 18, 2022
View reviewed changes

@sethkfman sethkfman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not blockers but I have a number of suggestions as things progress in this area.

Comment thread app/core/RPCMethods/utils.ts
Comment thread app/core/RPCMethods/RPCMethodMiddleware.ts Outdated
Comment thread app/core/RPCMethods/RPCMethodMiddleware.ts
@github-actions

github-actions Bot commented Jan 18, 2022

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Cal-L
Cal-L reviewed Jan 18, 2022
View reviewed changes

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I pushed a temp fix for TS errors and left some comments.

Comment thread app/core/RPCMethods/RPCMethodMiddleware.ts
Comment thread app/core/RPCMethods/RPCMethodMiddleware.ts
Comment thread app/core/RPCMethods/RPCMethodMiddleware.ts
@gantunesr gantunesr mentioned this pull request Jan 20, 2022
Closed
@andrepimenta andrepimenta changed the base branch from develop to main January 26, 2022 18:35
cortisiko
cortisiko approved these changes Jan 26, 2022
View reviewed changes

@cortisiko cortisiko 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.

🌮 🌮 🌮

@andrepimenta andrepimenta merged commit 375f742 into main Jan 27, 2022
@andrepimenta andrepimenta deleted the fix/RPCMethods-refactor branch January 27, 2022 09:23
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Cal-L Cal-L Cal-L left review comments

@cortisiko cortisiko cortisiko approved these changes

@gantunesr gantunesr Awaiting requested review from gantunesr

+2 more reviewers

@rickycodes rickycodes rickycodes left review comments

@sethkfman sethkfman sethkfman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@andrepimenta andrepimenta

Labels

needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Priority - High Task with high priority QA Passed QA testing has been completed and passed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@andrepimenta @cortisiko @rickycodes @sethkfman @Cal-L @gantunesr @mobularay