Update wallet keymanager for v0.11 #5453
Conversation
|
Latest version of this patch uses the local third party version of herumi, so should have no conflicts. An interop run confirms that signing is carried out and verifies as expected. |
There was a problem hiding this comment.
A few feedback comments. We expect to have a design ready to review for #5423 by EOD today or tomorrow.
| @@ -143,8 +143,5 @@ go_library( | |||
| }), | |||
| copts = OPTS, | |||
| cgo = True, | |||
| visibility = [ | |||
| # Additional access will require security approval. | |||
| "@prysm//shared/bls:__pkg__", | |||
There was a problem hiding this comment.
Please do not remove this. We can accept another whitelisted target for now, but ultimately want everything to use //shared/bls:go_default_library.
There was a problem hiding this comment.
Apologies; I couldn't find a way of doing this explicitly without creating a dependency loop. With the changes made elsewhere in the patch this now works with an explicit package name.
WORKSPACE
Outdated
| url = "https://github.com/wealdtech/go-eth2-types/archive/2245f15f56405852a0dc040864f6e8570735b04a.zip", | ||
| sha256 = "95a166c015e1a25ed53421807b8cc382fde3c414bae6f3ad8bcc3cb511cd77cb", | ||
| strip_prefix = "go-eth2-types-2245f15f56405852a0dc040864f6e8570735b04a", | ||
| build_file = "@prysm//third_party:eth2_types.BUILD" |
There was a problem hiding this comment.
Instead of a build_file, you can use build_directives to specify the herumi library alias with a go_repository.
| build_file = "@prysm//third_party:eth2_types.BUILD" | |
| build_directives = [ | |
| "gazelle:resolve go github.com/herumi/bls-eth-go-binary/bls @herumi_bls_eth_go_binary//:go_default_library", | |
| ], |
Note: this suggestion requires changing from http_archive to go_repository.
There was a problem hiding this comment.
Great stuff; this seems to be a lot cleaner. My bazel isn't up to much, I'm afraid.
third_party/eth2_types.BUILD
Outdated
| importpath_aliases = ["github.com/wealdtech/go-eth2-types"], | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| "@herumi_bls_eth_go_binary//:go_default_library", |
There was a problem hiding this comment.
We're going to want to remove this dependency somehow. We're still working on revamp design for integrating the functionality of wallet / keymanager, but I am thinking we can agree on some interface that would be implemented by //shared/bls:go_default_library.
There was a problem hiding this comment.
No problems with doing this once it's available. May be worth raising as a separate issue, rather than holding back this patch for it, given that any users on the current testnet who are using the wallet keymanager won't be able to sign correctly without this patch in place and the testnet goes live in around 24 hours.
There was a problem hiding this comment.
Agreed on that. No delay for this PR. Thanks!
WORKSPACE
Outdated
| version = "v2.3.0", | ||
| build_directives = [ | ||
| "gazelle:resolve go github.com/herumi/bls-eth-go-binary/bls @herumi_bls_eth_go_binary//:go_default_library", | ||
| ] |
There was a problem hiding this comment.
I think the linter is going to complain about a missing trailing comma.
This is an attempt to re-apply #4984 to allow keys held by the wallet keymanager to be able to sign in the new testnet.
The patch works as-is, however there have been changes to herumi within prysm since the original patch went in. As such I have had to re-add Nishant's herumi installation as a workspace item.
I assume there is some way around this, using the herumi library that's been introduced as a third-party element, but I'm unaware of how to do so. Any guidance on this would be much appreciated.