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

Reintroduce keymanager updates / improvements / changes from v0.11 #5423

Closed
4 tasks
prestonvanloon opened this issue Apr 14, 2020 · 4 comments
Closed
4 tasks
Assignees
Labels
Priority: High High priority item

Comments

@prestonvanloon
Copy link
Member

During updates to v0.11 branch, we decided to revert a few feature PRs to facilitate merging #5127 into master. This PR is tracking reintroduction of the feature, improvements, and updates from @mcdee.

I'll own this as a high priority item this week and work with @mcdee to ensure these changes land in master. There was some offline discussion about rationale behind the reversions and ultimately we want to accept these features while unblocking everyone in development.

One of the key things that was pointed out that there exists a reimplementation of several Prysm / eth2 methods in github.com/wealdtech/go-eth2-types due to the organization and dependencies of a large project like Prysm. We've also heard from other downstream users (#5130) that Prysm is difficult to build upon due to it's project structure and use of patches, particularly related to ethereumapis. Given that github.com/prysmaticlabs/ethereumapis is unlikely to be accepted as a canonical source for ethereum API definitions, we should reconsider our patch approach and general avoidance on ssz specific information in ethereum APIs.

Lastly, I would like to reevaluate Prysm's dependency management to leverage go modules to Prysm more available to downstream users. I'll follow up with a design proposal for reorganizing Prysm, its dependencies, and certain helpful types for downstream users.

@prestonvanloon prestonvanloon added the Priority: High High priority item label Apr 14, 2020
@prestonvanloon prestonvanloon self-assigned this Apr 14, 2020
@mbucci
Copy link

mbucci commented Apr 15, 2020

@prestonvanloon RE: dependency management and Go modules. If you're not already aware, Bazel supports Go Modules with Gazelle. For example, I've been using

bazel run //:gazelle -- update-repos -from_file=go.mod

to integrate a fork of go-eth2-wallet. This approach is still susceptible to the patches you mentioned though.

@prestonvanloon
Copy link
Member Author

From @mcdee

@prestonvanloon regarding the individal PRs in there:

  • 5260 was the update to 0.10; I opened another PR for 0.11 but it wasn't merged. Easiest thing to do here is to open a new PR that has both sets of changes, I think. If you concur I'll send one in
  • 5236 there are already people using this keymanager in the new testnet, so no longer suitable to remove, and ditto 5249
  • 4984 was an update for the 0.11 functionality. It probably needs to be refreshed given our conversation about the implicit init() that was in go-eth2-types with a newer version of the type (and again as per our discussion would love to be able to replace go-eth2-types entirely with a prysm repo once it gets broken out)

https://discordapp.com/channels/476244492043812875/550365651118850081/699998610204655627

@mcdee
Copy link
Contributor

mcdee commented Apr 16, 2020

#5453 is a reimplementation of #4984 that is suitable for current master.

Note that this PR is required before wallet keymanagers will be able to create valid signatures for anything in the current testnet.

@rauljordan
Copy link
Contributor

Closing in favor of #6220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority item
Projects
None yet
Development

No branches or pull requests

4 participants