-
Notifications
You must be signed in to change notification settings - Fork 82
feat: remove duplication of web3 client verrsion method #3508
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: remove duplication of web3 client verrsion method #3508
Conversation
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Test Results 18 files - 4 236 suites - 44 33m 8s ⏱️ - 26m 16s Results for commit 2f687e9. ± Comparison against base commit 00d411b. This pull request removes 5 and adds 23 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
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.
LGTM
Approved but however, we should also reach out to DAs to make sure this removal is not blocking anyone |
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.
Out of curiosity, do we know why we added it in the first place?
Signed-off-by: nikolay <n.atanasow94@gmail.com>
TBH I don't know. I tried to dig deeper but it was added a long time ago (September 2022) and I didn't find any clue. |
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.
LGTM
Signed-off-by: nikolay <n.atanasow94@gmail.com>
2f687e9
|
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.
LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3508 +/- ##
==========================================
+ Coverage 85.38% 85.55% +0.16%
==========================================
Files 74 74
Lines 4777 4777
Branches 985 985
==========================================
+ Hits 4079 4087 +8
+ Misses 407 403 -4
+ Partials 291 287 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
* chore: remove web3_client_version Signed-off-by: nikolay <n.atanasow94@gmail.com> * chore: remove duplicated test Signed-off-by: nikolay <n.atanasow94@gmail.com> --------- Signed-off-by: nikolay <n.atanasow94@gmail.com>
Description:
Currently, there are two implementations of the web3 client version method.
Solution:
Research why we've added both of them (is the first one deprecated) and remove the non-standard one.
Related issue(s):
Fixes #3505
Notes for reviewer:
After some research, we found out that there is no
web3_client_version
in any JSON RPC standard even in the deprecated ones.Checklist