8000 feat(jsonrpc): add support for allow-insecure-unlock by timwhite2 · Pull Request #2375 · evmos/evmos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(jsonrpc): add support for allow-insecure-unlock #2375

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 30 commits into from
May 13, 2024
Merged

feat(jsonrpc): add support for allow-insecure-unlock #2375

merged 30 commits into from
May 13, 2024

Conversation

timwhite2
Copy link
Contributor

Allow insecure account unlocking when account-related RPCs are exposed by http

Description

Closes: #1657

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the correct branch (see PR Targeting)

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code
  • reviewed content
  • tested instructions (if applicable)
  • confirmed all CI checks have passed

@timwhite2
feat: impl geth allow-insecure-unlock
8000
abec233 
Allow insecure account unlocking when account-related RPCs are exposed by http
@timwhite2 timwhite2 requested a review from a team as a code owner February 23, 2024 08:57
@timwhite2 timwhite2 requested review from 0xstepit and GAtom22 and removed request for a team February 23, 2024 08:57
@0xstepit
Copy link
Contributor

Hey @timwhite2, this is cool! Thanks for the implementation of this feature. Will look into it ASAP

@codecov Codecov
Copy link
codecov bot commented Feb 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 25.00000% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 59.55%. Comparing base (f097735) to head (738e2f6).
Report is 145 commits behind head on main.

❗ Current head 738e2f6 differs from pull request most recent head d642c59. Consider uploading reports for the commit d642c59 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2375       +/-   ##
===========================================
- Coverage   70.45%   59.55%   -10.90%     
===========================================
  Files         293      341       +48     
  Lines       22559    21598      -961     
===========================================
- Hits        15893    12862     -3031     
- Misses       5800     7689     +1889     
- Partials      866     1047      +181
Files Coverage Δ
server/config/config.go 47.05% <100.00%> (+10.24%) ⬆️
server/flags/flags.go 71.42% <ø> (ø)
server/start.go 19.89% <100.00%> (ø)
rpc/backend/sign_tx.go 57.14% <0.00%> (-3.81%) ⬇️
rpc/backend/node_info.go 43.42% <0.00%> (-3.09%) ⬇️
< 8000 p dir="auto">... and 341 files with indirect coverage changes

GAtom22
GAtom22 reviewed Feb 29, 2024
View reviewed changes
Copy link
Contributor
@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @timwhite2!!

Left one comment.
Also, please add a changelog entry

GAtom22 and others added 3 commits February 29, 2024 17:51
@GAtom22
Merge branch 'main' into feat-jsonrpc-allow-insecure-unlock
a8aaa3c
@timwhite2
remove JSONRPC.Enable check
f340c57 
1.remove JSONRPC.Enable check
2.add changelog
3.modify JSONRPC.Enable description
@timwhite2
change log type
7a8066e
GAtom22
GAtom22 approved these changes Mar 1, 2024
View reviewed changes
Copy link
Contributor
@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

LGTM!! Thanks for this great contribution @timwhite2!

GAtom22
GAtom22 reviewed Mar 1, 2024
View reviewed changes
GAtom22
GAtom22 reviewed Mar 1, 2024
View reviewed changes
@CLAassistant
Copy link
CLAassistant commented Mar 4, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

ramacarlucho
ramacarlucho previously approved these changes Mar 7, 2024
View reviewed changes
Copy link
Contributor
@ramacarlucho ramacarlucho left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for the contribution

@ramacarlucho ramacarlucho dismissed their stale review March 7, 2024 12:26

Test not passing

ramacarlucho
ramacarlucho reviewed Mar 7, 2024
View reviewed changes
ramacarlucho
ramacarlucho previously requested changes Mar 7, 2024
View reviewed changes
Copy link
Contributor
@ramacarlucho ramacarlucho left a comment

Choose a reason for hiding this comment

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

Can you fix the tests that are not passing?

@timwhite2
Copy link
Contributor Author

Can you fix the tests that are not passing?

Sure

@timwhite2
Copy link
Contributor Author

The other errors seem to have nothing to do with this branch

@timwhite2
Copy link
Contributor Author

@ramacarlucho It seems that all required checks have passed

MalteHerrmann
MalteHerrmann previously requested changes Mar 28, 2024
View reviewed changes
Copy link
Contributor
@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, however I see it's being enabled in a lot of places but it's usage is actually not tested. There should be dedicated tests added to showcase the feature and that it is working, if we're going to enable it.

timwhite2 and others added 4 commits March 29, 2024 09:36
@timwhite2 @MalteHerrmann
Update server/config/config.go
66177ec 
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
@timwhite2
Merge branch 'main' into feat-jsonrpc-allow-insecure-unlock
2f3d06c
@GAtom22
Merge branch 'main' into feat-jsonrpc-allow-insecure-unlock
dd849d2 
Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
@GAtom22 @MalteHerrmann
Update local_node.sh
1b8a9f1 
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
@GAtom22 GAtom22 requested a review from MalteHerrmann May 13, 2024 14:09
@GAtom22 GAtom22 merged commit 0c5f7db into evmos:main May 13, 2024
@0xstepit
Copy link
Contributor

@Mergifyio backport release/v18.0.x

@mergify Mergify
Copy link
Contributor
mergify bot commented May 27, 2024 •
edited
Loading

backport release/v18.0.x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 27, 2024
@timwhite2 @mergify
feat(jsonrpc): add support for allow-insecure-unlock (#2375)
6aca615 
* feat: impl geth allow-insecure-unlock

Allow insecure account unlocking when account-related RPCs are exposed by http

* fix: Solidity Test error

* remove JSONRPC.Enable check

1.remove JSONRPC.Enable check
2.add changelog
3.modify JSONRPC.Enable description

* change log type

* Update CHANGELOG.md

* fix lint issues

* fix test-nix error

* fix: test error

* Update init-node.sh

* Update CHANGELOG.md

Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com>

* fix: solidity test error

* fix: rpc test error

* Update server/config/config.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update local_node.sh

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>

* remove duplicated file

* set default value to true to ensure backwards compatibility

* remove unnecessary change

---------

Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: tom <tomasguerraalda@hotmail.com>
(cherry picked from commit 0c5f7db)

# Conflicts:
#	CHANGELOG.md
@mergify mergify bot mentioned this pull request May 27, 2024
Closed
9 tasks
facs95 added a commit that referenced this pull request May 29, 2024
@timwhite2 @0xstepit
@GAtom22 @facs95 @ramacarlucho @MalteHerrmann
feat(jsonrpc): add support for allow-insecure-unlock (#2375)
ff9709e 
* feat: impl geth allow-insecure-unlock

Allow insecure account unlocking when account-related RPCs are exposed by http

* fix: Solidity Test error

* remove JSONRPC.Enable check

1.remove JSONRPC.Enable check
2.add changelog
3.modify JSONRPC.Enable description

* change log type

* Update CHANGELOG.md

* fix lint issues

* fix test-nix error

* fix: test error

* Update init-node.sh

* Update CHANGELOG.md

Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com>

* fix: solidity test error

* fix: rpc test error

* Update server/config/config.go

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>

* Update local_node.sh

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>

* remove duplicated file

* set default value to true to ensure backwards compatibility

* remove unnecessary change

---------

Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: tom <tomasguerraalda@hotmail.com>
GAtom22 added a commit that referenced this pull request May 29, 2024
@facs95 @timwhite2
@0xstepit @GAtom22 @ramacarlucho @MalteHerrmann
feat(jsonrpc): add support for allow-insecure-unlock (#2375) (#2587)
ed8b493 
* feat: impl geth allow-insecure-unlock

Allow insecure account unlocking when account-related RPCs are exposed by http

* fix: Solidity Test error

* remove JSONRPC.Enable check

1.remove JSONRPC.Enable check
2.add changelog
3.modify JSONRPC.Enable description

* change log type

* Update CHANGELOG.md

* fix lint issues

* fix test-nix error

* fix: test error

* Update init-node.sh

* Update CHANGELOG.md



* fix: solidity test error

* fix: rpc test error

* Update server/config/config.go



* Update local_node.sh




* remove duplicated file

* set default value to true to ensure backwards compatibility

* remove unnecessary change

---------

Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: artik <100273990+timwhite2@users.noreply.github.com>
Co-authored-by: stepit <48993133+0xstepit@users.noreply.github.com>
Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: Ramiro Carlucho <ramirocarlucho@gmail.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: tom <tomasguerraalda@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GAtom22 GAtom22 GAtom22 approved these changes

@fedekunze fedekunze fedekunze approved these changes

@ramacarlucho ramacarlucho ramacarlucho left review comments

@0xstepit 0xstepit Awaiting requested review from 0xstepit 0xstepit is a code owner automatically assigned from evmos/core-engineering

@MalteHerrmann MalteHerrmann Awaiting requested review from MalteHerrmann

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem: keyring-backend test leading to accounts to be drained when 8545 exposed public
8 participants
@timwhite2 @0xstepit @CLAassistant @ramacarlucho @fedekunze @MalteHerrmann @GAtom22 @facs95
0