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

Check specified chain ID against endpoint return value #9491

Merged
merged 15 commits into from
Oct 7, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Oct 6, 2020

Adds additional validation to chainId values in the network form, by comparing the specified value against the value returned by the endpoint.

@rekmarks rekmarks requested a review from a team as a code owner October 6, 2020 21:51
@rekmarks rekmarks requested a review from whymarrh October 6, 2020 21:51
@whymarrh whymarrh requested review from Gudahtt and removed request for whymarrh October 6, 2020 21:59
whymarrh
whymarrh previously approved these changes Oct 6, 2020
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

Seems legit

Gudahtt
Gudahtt previously approved these changes Oct 6, 2020
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@rekmarks rekmarks dismissed stale reviews from Gudahtt and whymarrh via 911491d October 6, 2020 22:17
@rekmarks rekmarks requested review from Gudahtt and whymarrh October 6, 2020 22:27
Gudahtt
Gudahtt previously approved these changes Oct 6, 2020
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@rekmarks rekmarks force-pushed the network-form-chainId-validation branch from 886a77f to 69c71d4 Compare October 6, 2020 23:31
@rekmarks rekmarks force-pushed the network-form-chainId-validation branch from bfc5902 to 7af85c5 Compare October 7, 2020 00:20
@metamaskbot
Copy link
Collaborator

Builds ready [7af85c5]
Page Load Metrics (493 ± 87 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31121492110
domContentLoaded30997249118287
load31197349318287
domInteractive30997149118287

@metamaskbot
Copy link
Collaborator

Builds ready [be40a26]
Page Load Metrics (431 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3010243199
domContentLoaded32461442910349
load32661643110450
domInteractive32461442910350

Gudahtt
Gudahtt previously approved these changes Oct 7, 2020
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@rekmarks rekmarks force-pushed the network-form-chainId-validation branch from fb2983a to 857cfbe Compare October 7, 2020 03:07
@metamaskbot
Copy link
Collaborator

Builds ready [e376823]
Page Load Metrics (448 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint307943157
domContentLoaded31567044712459
load31667144812459
domInteractive31567044612459

Comment on lines +421 to +434
const jsonRpcResponse = await window.fetch(rpcUrl, {
method: 'POST',
body: JSON.stringify({
id: Date.now().toString(),
jsonrpc: '2.0',
method: rpcMethod,
params: rpcParams,
}),
headers: {
'Content-Type': 'application/json',
},
cache: 'default',
})
.then((httpResponse) => httpResponse.json())
Copy link
Member

Choose a reason for hiding this comment

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

Nit: .then => async/await

Suggested change
const jsonRpcResponse = await window.fetch(rpcUrl, {
method: 'POST',
body: JSON.stringify({
id: Date.now().toString(),
jsonrpc: '2.0',
method: rpcMethod,
params: rpcParams,
}),
headers: {
'Content-Type': 'application/json',
},
cache: 'default',
})
.then((httpResponse) => httpResponse.json())
const httpResponse = await window.fetch(rpcUrl, {
method: 'POST',
body: JSON.stringify({
id: Date.now().toString(),
jsonrpc: '2.0',
method: rpcMethod,
params: rpcParams,
}),
headers: {
'Content-Type': 'application/json',
},
cache: 'default',
})
const jsonRpcResponse = await httpResponse.json()

* @param {string} rpcUrl - The RPC endpoint URL to target.
* @param {string} rpcMethod - The RPC method to request.
* @param {Array<unknown>} [rpcParams] - The RPC method params.
* @returns {Promise<unknown|undefined>} Returns the result of the RPC method call,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: unknown|undefined seems redundant 🤔 since undefined is a valid unknown value

Suggested change
* @returns {Promise<unknown|undefined>} Returns the result of the RPC method call,
* @returns {Promise<unknown>} Returns the result of the RPC method call,

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! Couple of small suggestions remain, but nothing blocking.

@Gudahtt Gudahtt merged commit bf1bb6c into develop Oct 7, 2020
@Gudahtt Gudahtt deleted the network-form-chainId-validation branch October 7, 2020 14:28
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2020
@Gudahtt
Copy link
Member

Gudahtt commented Oct 7, 2020

I decided to merge this as-is so that we can get another RC up without further delay

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

Successfully merging this pull request may close these issues.

4 participants