-
Notifications
You must be signed in to change notification settings - Fork 5k
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
I#5956 fix2 dont overwrite existing rpc settings #6044
Conversation
@frankiebee does this also fix #5934 |
const frequentRpcListDetail = this.preferencesController.getFrequentRpcListDetail() | ||
const rpcSettings = frequentRpcListDetail.find((rpc) => rpcTarget === rpc.rpcUrl) | ||
|
||
if (rpcSettings) { |
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.
I think this will break the ability to update an existing rpc url. I mean, currently if a user reenters the details of an rpc url, it will update that rpc url's data. I am note sure if we care about that, but this change will break that ability.
We could add an "updateCustomRpc" method that is called if a user enters an existing rpc url in the form on the settings page.
Code looks good and it does fix the linked issue. Sort of breaks another ability of the custom rpc form that might be used. We should either agree that it is okay to break that, or fix it in this PR. |
I don't think that has to block this, considering this fixes a pretty big issue, but I agree, it would be better if we never broke any behavior in the first place, and don't mind waiting an extra day or two for that. What do you think, @frankiebee? |
yeah i can get that in |
…dateAndSetCustomRpc
Thanks everyone who is working on it. I highly appreciate it. |
ec994e1
to
38e5791
Compare
adding the update function needs some more work |
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.
Reviewed code and QA'd. Looks good. Approved.
fixes #5956