fix: wallet_addEthereumChain does not attach a result under certain conditions#26726
fix: wallet_addEthereumChain does not attach a result under certain conditions#26726
wallet_addEthereumChain does not attach a result under certain conditions#26726Conversation
…he response object when the currently selected rpcUrl matches the request
|
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. |
|
This change looks correct to me, but I am curious how it might have caused this problem. Wouldn't |
I would have thought so too... which is certainly why this bug was introduced and unnoticed. And even when debugging I stared at it for a while thinking this should resolve the request. But apparently the rpc engine needs a result if an error is not specified in the end function... something to probably look into modifying in the JSON RPC Engine |
|
Oh I see, I recall that now. Makes sense. |
|
seems like the chrome e2e test failure might relate to these brownouts? All the tests seem to be passing... 
|
| currentChainIdForDomain === chainId && | ||
| currentRpcUrl === firstValidRPCUrl | ||
| ) { | ||
| res.result = null; |
There was a problem hiding this comment.
Not a blocker, but curious how the JsonRpcEngine didn't end up throwing for this. AFAIK if we process all middlewares without a result we end up throwing an error?
There was a problem hiding this comment.
It did throw an error in this case, and it was returned to the dapp. But the QueuedRequestController missed it for some reason.
But yeah, good point, how was this missed 🤔
There was a problem hiding this comment.
It actually did throw an error to the dapp as you can see in the after video
There was a problem hiding this comment.
But the QueuedRequestController missed it for some reason.
Yeah... we should investigate this. I can create a ticket
There was a problem hiding this comment.
|
Possibly; the initial failure is one we've always seen occasionally on CircleCI. The "Rerun failed tests" button always fails unless test artifacts have been uploaded during the failed run, so "Rerun failed workflows" needs to be used instead. I've just triggered that. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26726 +/- ##
===========================================
- Coverage 70.09% 70.08% -0.01%
===========================================
Files 1413 1413
Lines 49255 49318 +63
Branches 13768 13780 +12
===========================================
+ Hits 34524 34562 +38
- Misses 14731 14756 +25 ☔ View full report in Codecov by Sentry. |
Builds ready [2cf7859]
Page Load Metrics (71 ± 7 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| jest.spyOn(chainUtils, 'findExistingNetwork').mockReturnValue({ | ||
| chainId: '0x1', | ||
| rpcUrl: 'https://mainnet.infura.io/v3/', | ||
| ticker: 'ETH', | ||
| }); |
There was a problem hiding this comment.
guessing you can just modify the findNetworkConfigurationBy mock result because findExistingNetwork also checks the built in networks constant?
if so, can we modify this test to use a non-built-in network instead?
There was a problem hiding this comment.
if it wasnt* clear, this is a nit and totally ignorable. I'm mainly asking so we can avoid having to import and mock chainutils
jiexi
left a comment
There was a problem hiding this comment.
approving to avoid being a blocker. feel free to ignore nit
|
Builds ready [c4c185b]
Page Load Metrics (82 ± 10 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Fix issue where
wallet_addEthereumChaindoes not attach a result to the response object when the currently selected rpcUrl matches the request.This would cause the request to get "stuck" in the
QueuedRequestControllerqueue, preventing the queue from progressing and causing confusing behavior.Related issues
Fixes: #26706
Manual testing steps
(For other chains it appears Chainlist cycles to the next rpcUrl you don't have which avoids this bug, so use Blast)
Screenshots/Recordings
Before
Screen.Recording.2024-08-28.at.1.37.50.PM.mov
After
Screen.Recording.2024-08-28.at.1.33.16.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist