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

Prysm V4: Remove Prysm Remote Signer #11895

Merged
merged 33 commits into from Mar 9, 2023
Merged

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented Jan 19, 2023

What type of PR is this?

Remove Remote Signer

What does this PR do? Why is it needed?

Prysm Remote Signer hasn't been supported well since last year before the merge. All client teams have since switched to supporting the web3signer interface. This PR attempts to fully remove the Prysm Remote Signer which is gRPC based from the repo clearing up some logic required in the setup.

Depends on #11857
Part of #11841

Which issues(s) does this PR fix?

Fixes #11700

@james-prysm james-prysm added the Cleanup Code health! label Jan 19, 2023
@james-prysm james-prysm self-assigned this Jan 19, 2023
@james-prysm james-prysm added the Blocked Blocked by research or external factors label Jan 19, 2023
@potuz
Copy link
Contributor

potuz commented Jan 20, 2023

Unless it's completely distinctional and dangerous to use, can we wait on this one a few weeks to see if someone voices over?
Cc @prestonvanloon

@james-prysm james-prysm changed the title Depricate Prysm Remote Signer Prysm V4: Depricate Prysm Remote Signer Jan 20, 2023
@james-prysm
Copy link
Contributor Author

@potuz yes we can wait, this depends on web3signer tls as well. would be good for v4 though.

@james-prysm james-prysm added the Breaking Changes Breaking changes for a major release label Jan 20, 2023
@nisdas nisdas changed the title Prysm V4: Depricate Prysm Remote Signer Prysm V4: Deprecate Prysm Remote Signer Feb 1, 2023
@james-prysm james-prysm marked this pull request as ready for review February 9, 2023 15:08
@james-prysm james-prysm requested a review from a team as a code owner February 9, 2023 15:08
Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

The code looks good to me. One question I have is how early we should be warning users about this type of breaking change. Typically we should give them at least a release or more advanced time to prepare. That it goes from deprecate warning -> fully delete

@james-prysm
Copy link
Contributor Author

The code looks good to me. One question I have is how early we should be warning users about this type of breaking change. Typically we should give them at least a release or more advanced time to prepare. That it goes from deprecate warning -> fully delete

unlike the webui i don't think anyone is currently using this, as it hasn't been updated since before the merge and we only offered a prototype implementation. so I think coinbase was the last user of this feature.
#11700 also opened this in november. I wanted to defer to preston if he knew more but if we want to put another notice before merging I am ok with that decision too...

@james-prysm james-prysm changed the title Prysm V4: Deprecate Prysm Remote Signer Prysm V4: Remove Prysm Remote Signer Feb 21, 2023
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

Ok with this is v4 is the next release. Otherwise, we might want to make sure to deprecate instead of deleting

@james-prysm james-prysm merged commit 753e285 into develop Mar 9, 2023
@delete-merged-branch delete-merged-branch bot deleted the depricate-remote-signer branch March 9, 2023 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Blocked by research or external factors Breaking Changes Breaking changes for a major release Cleanup Code health!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate the Prysm Remote Signer in favor of Web3signer interface
4 participants