-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
feat: added test network as selected network if globally selected for connection Request #27980
base: develop
Are you sure you want to change the base?
Conversation
@@ -240,37 +222,19 @@ describe('Request Queuing Dapp 1, Switch Tx -> Dapp 2 Send Tx', function () { | |||
`window.ethereum.request(${switchEthereumChainRequest})`, | |||
); | |||
|
|||
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Dapp is already connected to Test Network, we don't need to show review permission screem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the goal of the switch call is to trigger an approval to permit a chain that is not already permitted. This check still needs to occur. Instead the switch call above to should target 0x53a It may be easier to unselect 0x539 (Localhost 8545) when Dapp One is connecting
Builds ready [0355cf1]
Page Load Metrics (2039 ± 204 ms)
Bundle size diffs
|
006e40d
to
0355cf1
Compare
Builds ready [aec3385]
Page Load Metrics (2213 ± 106 ms)
Bundle size diffs
|
const editButtons = await this.driver.findElements( | ||
'[data-testid="edit"]', | ||
); | ||
await editButtons[1].click(); | ||
|
||
await this.driver.clickElement({ | ||
text: 'Localhost 8545', | ||
tag: 'p', | ||
}); | ||
|
||
await this.driver.clickElement( | ||
'[data-testid="connect-more-chains-button"]', | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed anymore since testnet is now included in the connections request
Builds ready [a18a31d]
Page Load Metrics (1737 ± 127 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General logic looks good to me. Would appreciate if someone else reviews the E2E changes in detail
const selectedTestNetwork = testNetworks.filter( | ||
(network: { chainId: string }) => | ||
network.chainId === currentlySelectedNetworkChainId, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should only be one network that matches the chainId, does that sound right to you? We may want to use .find()
here to make that clearer if so
); | ||
|
||
const selectedNetworksList = | ||
selectedTestNetwork === undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear to me how selectedTestNetwork
would ever be undefined if it's the result of testNetworks.filter()
which should always return an array
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); | ||
await driver.findElement({ | ||
text: 'Use your enabled networks', | ||
tag: 'p', | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check should still be valid. The globally selected network is 0x53a when Dapp Two connects, meaning that 0x539 should not be connected for Dapp Two and a Chain Permission approval should appear when trying to switch to it
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); | ||
|
||
// Check correct network on the send confirmation. | ||
await driver.findElement({ | ||
css: '[data-testid="network-display"]', | ||
text: 'Localhost 8546', | ||
}); | ||
|
||
await driver.clickElementAndWaitForWindowToClose({ | ||
text: 'Confirm', | ||
tag: 'button', | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should also still be valid, but I would assume the confirmation is on Localhost 8545
await driver.switchToWindowWithUrl(DAPP_ONE_URL); | ||
|
||
await driver.clickElement('#sendButton'); | ||
|
||
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); | ||
|
||
await driver.clickElement({ text: 'Cancel', tag: 'button' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to stay the same as well since we are testing the scenario when there is an approval for permitting a chain
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); | ||
|
||
// Check correct network on the send confirmation. | ||
await driver.findElement({ | ||
css: '[data-testid="network-display"]', | ||
text: 'Localhost 8546', | ||
}); | ||
|
||
await driver.clickElementAndWaitForWindowToClose({ | ||
text: 'Confirm', | ||
tag: 'button', | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should stay as well. The goal here is to confirm that the network switch on Dapp One did not affect Dapp Two
// Switch to Dapp One and connect it | ||
await driver.switchToWindowWithUrl(DAPP_URL); | ||
await driver.clickElement({ text: 'Connect', tag: 'button' }); | ||
|
||
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); | ||
|
||
await driver.clickElementAndWaitForWindowToClose({ | ||
text: 'Connect', | ||
tag: 'button', | ||
}); | ||
|
||
// Switch to MM and disconnect localhost 8545 for Dapp One | ||
|
||
await driver.switchToWindowWithTitle( | ||
WINDOW_TITLES.ExtensionInFullScreenView, | ||
); | ||
await driver.clickElement( | ||
'[data-testid ="account-options-menu-button"]', | ||
); | ||
await driver.clickElement({ | ||
text: 'All Permissions', | ||
tag: 'div', | ||
}); | ||
await driver.clickElementAndWaitToDisappear({ | ||
text: 'Got it', | ||
tag: 'button', | ||
}); | ||
await driver.clickElement({ | ||
text: '127.0.0.1:8080', | ||
tag: 'p', | ||
}); | ||
const editButtons = await driver.findElements('[data-testid="edit"]'); | ||
|
||
assert.ok(editButtons.length > 0, 'Edit buttons are available'); | ||
|
||
await editButtons[1].click(); | ||
|
||
// Disconnect Mainnet | ||
await driver.clickElement({ | ||
text: 'Localhost 8546', | ||
tag: 'p', | ||
}); | ||
|
||
await driver.clickElement('[data-testid="connect-more-chains-button"]'); | ||
// Switch to Dapp Two | ||
await driver.switchToWindowWithUrl(DAPP_ONE_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts on the connection happening right after line 147? You also had a nice pattern in the other test of deselecting a network during the connection flow which would be great to use here instead of opening the wallet and disconnecting the chain after connecting it
await driver.switchToWindowWithTitle( | ||
WINDOW_TITLES.ExtensionInFullScreenView, | ||
); | ||
await driver.clickElement( | ||
'[data-testid ="account-options-menu-button"]', | ||
); | ||
await driver.clickElement({ text: 'All Permissions', tag: 'div' }); | ||
await driver.clickElementAndWaitToDisappear({ | ||
text: 'Got it', | ||
tag: 'button', | ||
}); | ||
await driver.clickElement({ | ||
text: '127.0.0.1:8081', | ||
tag: 'p', | ||
}); | ||
const editButtons = await driver.findElements('[data-testid="edit"]'); | ||
|
||
// Ensure there are edit buttons | ||
assert.ok(editButtons.length > 0, 'Edit buttons are available'); | ||
|
||
// Click the first (0th) edit button | ||
await editButtons[1].click(); | ||
|
||
// Disconnect Mainnet | ||
await driver.clickElement({ | ||
text: 'Localhost 8545', | ||
tag: 'p', | ||
}); | ||
|
||
await driver.clickElement('[data-testid="connect-more-chains-button"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, would be nice to just not connect Dapp Two to 0x539 during the connection flow itself rather than disconnecting it afterwards
await driver.switchToWindowWithTitle( | ||
WINDOW_TITLES.ExtensionInFullScreenView, | ||
); | ||
await driver.clickElement( | ||
'[data-testid ="account-options-menu-button"]', | ||
); | ||
await driver.clickElement({ text: 'All Permissions', tag: 'div' }); | ||
await driver.clickElementAndWaitToDisappear({ | ||
text: 'Got it', | ||
tag: 'button', | ||
}); | ||
await driver.clickElement({ | ||
text: '127.0.0.1:8081', | ||
tag: 'p', | ||
}); | ||
const editButtons = await driver.findElements('[data-testid="edit"]'); | ||
|
||
// Ensure there are edit buttons | ||
assert.ok(editButtons.length > 0, 'Edit buttons are available'); | ||
|
||
// Click the first (0th) edit button | ||
await editButtons[1].click(); | ||
|
||
// Disconnect Mainnet | ||
await driver.clickElement({ | ||
text: 'Localhost 8545', | ||
tag: 'p', | ||
}); | ||
|
||
await driver.clickElement('[data-testid="connect-more-chains-button"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
This PR is to select a test network in the default selected networks list if its the globally selected network at the time of connection request.
Related issues
Fixes: #27891
Manual testing steps
Screenshots/Recordings
Before
Screen.Recording.2024-10-21.at.3.09.20.PM.mov
After
Screen.Recording.2024-10-21.at.3.07.23.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist