Web Backend Revert delete rpc #8681

ahadda5 · 2021-03-28T14:33:23Z

What type of PR is this?
Feature #8664

What does this PR do? Why is it needed?
WebUI needs to allow for the deletion of accounts. We will revert back this feature which existed before.

Which issues(s) does this PR fix?
Fixes #8664 The portion about adding back the delete accounts RPC endpoints, previously reverted from Prysm

Other notes for review
On reverting the code the following considerations were accounted for.

Added The DeleteAccountRequest and Response with the DeletedPublicKeys in protobuf
InitKeymanagerConfig func used in the DeleteAccounts is called with ListenForChanges not SkipMnemonic as before
Added back a func createImportedWalletWithAccounts , originally part of wallet_test.go which , as the name implies, creates a wallet and imports a test-generated keystore.

…ys provided and the successful imported account with provided public keys; also brought back the createImportedWalletWithAccounts back in wallet_test.go

… webbackendrevertDeleteRPC

rkapka

Please run goimports. We should have local imports first, followed by an empty line, followed by external imports. Changes to most pb files' imports are unnecessary.

rkapka · 2021-03-29T07:22:39Z

validator/rpc/accounts.go

+func (s *Server) DeleteAccounts(
+	ctx context.Context, req *pb.DeleteAccountsRequest,
+) (*pb.DeleteAccountsResponse, error) {
+	if req.DeletePublicKeys == nil || len(req.DeletePublicKeys) < 1 {


It would be good to also add a test for this precondition.

validator/rpc/accounts_test.go

rkapka · 2021-03-29T07:24:24Z

validator/rpc/accounts_test.go

+}
+
+func TestServer_DeleteAccounts_OK(t *testing.T) {
+	ss, pubKeys := createImportedWalletWithAccounts(t, 3)


ss is a really bad name. Can we change it to server?

rkapka · 2021-03-29T07:25:08Z

validator/rpc/wallet_test.go

+	//require.NoError(t, w.SaveHashedPassword(ctx))
+	km, err := w.InitializeKeymanager(ctx, iface.InitKeymanagerConfig{ListenForChanges: false})
+	require.NoError(t, err)
+	ss := &Server{


ss is a weird name. Why not s or server?

changed to s

rkapka · 2021-03-29T07:27:09Z

proto/validator/accounts/v2/web_api.proto

+
+message DeleteAccountsRequest {
+    // List of public keys to delete.
+    repeated bytes delete_public_keys = 1;


delete_public_keys sounds like a good function name, but parameter names should be nouns. I would prefer simply public_keys.

agreed. Although, I was trying to be consistent with the accounts.Config . Should i change the protobuf?

type Config struct { | Wallet *wallet.Wallet | Keymanager keymanager.IKeymanager | DeletePublicKeys [][]byte | }

It's OK if they are not the same. Config's name is also a bit awkward, I think PublicKeysToDelete makes more sense (I would not call it simply PublicKeys because the type Config does not convey any information as to what these public keys are, as opposed to the protobuf type name).

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

…5/prysm into webbackendrevertDeleteRPC

ahadda5 · 2021-03-29T13:05:27Z

Please run goimports. We should have local imports first, followed by an empty line, followed by external imports. Changes to most pb files' imports are unnecessary.

AFter running goimports on the root proto libs , DeepSource complains about this below. What is that would you know?
var protoReq eth.PeerRequest

rkapka · 2021-03-29T13:21:37Z

So it looks like 7348624 removed an import from proto/beacon/rpc/v1_gateway/debug.pb.gw.go that was actually needed. Also the alias in import v1alpha1 "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" in proto/validator/accounts/v2_gateway/web_api.pb.go should be changed to eth (resulting in eth "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1").

…e deleterequest variable to PublicKeysToDelete

ahadda5 · 2021-03-29T13:42:17Z

I think its because i ran update proto scripts before goimports. It is fixed now. Thanks

rkapka · 2021-03-29T14:12:22Z

Unfortunately your last commit again mixed up imports. I am not sure why this happens. Can you maybe just revert these changes manually in case the tooling does not work properly?

rauljordan · 2021-03-30T02:12:41Z

validator/rpc/accounts.go

+	if s.wallet == nil || s.keymanager == nil {
+		return nil, status.Error(codes.FailedPrecondition, "No wallet found")
+	}
+	if s.wallet.KeymanagerKind() != keymanager.Imported {


We have since changed the requirements and now both imported and derived keymanagers can delete keys. Remote cannot, so we should update here

How would one test a derived wallet delete? accounts.DeleteAccounts supports both imported and derived but only tests the imported wallets (check here
If i get it to work in rpc , we can update the accounts module as well.

A derived wallet is internally implemented as an imported wallet, that's why testing an imported wallet is enough for the most part.

An example of setting up a derived wallet for testing can be found here:

prysm/validator/client/wait_for_activation_test.go

Line 311 in 28e4a3b

t.Run("Derived keymanager", func(t *testing.T) {

rauljordan

I think you might need to do gofmt -w -s . or goimports to prevent changing all those other unrelated proto files

rauljordan · 2021-03-30T02:13:01Z

validator/rpc/wallet_test.go

+		SkipMnemonicConfirm: true,
+	})
+	require.NoError(t, err)
+	//require.NoError(t, w.SaveHashedPassword(ctx))


Let's delete the commented code

… webbackendrevertDeleteRPC

…5/prysm into webbackendrevertDeleteRPC

…ported and derived

… webbackendrevertDeleteRPC

…5/prysm into webbackendrevertDeleteRPC

codecov · 2021-03-31T16:42:31Z

Codecov Report

Merging #8681 (3f7e724) into develop (28e4a3b) will increase coverage by 0.03%.
The diff coverage is 77.77%.

❗ Current head 3f7e724 differs from pull request most recent head b9eb9e4. Consider uploading reports for the commit b9eb9e4 to get more accurate results

@@             Coverage Diff             @@
##           develop    #8681      +/-   ##
===========================================
+ Coverage    61.15%   61.18%   +0.03%     
===========================================
  Files          498      498              
  Lines        34711    34729      +18     
===========================================
+ Hits         21227    21250      +23     
+ Misses       10332    10316      -16     
- Partials      3152     3163      +11

rauljordan · 2021-03-31T21:29:57Z

PTAL @rkapka

ahadda5 added 3 commits March 28, 2021 15:51

added the removed RPC delete account to accounts.go and the rpc in proto

8787fb2

reverted the 3 unit tests namely the failed derived delete, no pub ke…

aae6598

…ys provided and the successful imported account with provided public keys; also brought back the createImportedWalletWithAccounts back in wallet_test.go

strong password defined elsewhere- removed

1ee713e

ahadda5 requested a review from a team as a code owner March 28, 2021 14:33

ahadda5 requested review from farazdagi, 0xKiwi and nisdas March 28, 2021 14:33

Merge branch 'develop' of https://github.com/prysmaticlabs/prysm into…

d567635

… webbackendrevertDeleteRPC

rkapka requested changes Mar 29, 2021

View reviewed changes

Merge branch 'develop' into webbackendrevertDeleteRPC

d69b364

ahadda5 changed the title ~~Webbackendrevert delete rpc~~ Web Backend Revert delete rpc Mar 29, 2021

ahadda5 and others added 5 commits March 29, 2021 13:56

Update validator/rpc/accounts_test.go

618ed6e

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Merge branch 'webbackendrevertDeleteRPC' of https://github.com/ahadda…

521c6af

…5/prysm into webbackendrevertDeleteRPC

added PublicKeys Nil test case

d26b5e5

changed ss to s. ss was a bad name inthe first place

b3dc745

goimports -w root

7348624

fixed the goimports before running the proto scripts, also changed th…

ce53b7e

…e deleterequest variable to PublicKeysToDelete

rauljordan reviewed Mar 30, 2021

View reviewed changes

ahadda5 and others added 7 commits March 30, 2021 14:28

Merge branch 'develop' of https://github.com/prysmaticlabs/prysm into…

6f29428

… webbackendrevertDeleteRPC

removed unneeded comment

1c22d61

Merge branch 'develop' into webbackendrevertDeleteRPC

87a7f75

merge from origin

fe31a51

Merge branch 'webbackendrevertDeleteRPC' of https://github.com/ahadda…

f20da1a

…5/prysm into webbackendrevertDeleteRPC

Merge branch 'develop' into webbackendrevertDeleteRPC

0bc3918

added test case for derived, changed delete account to be for both im…

5af168a

…ported and derived

ahadda5 added 3 commits March 31, 2021 20:09

gofmt

b836085

Merge branch 'develop' of https://github.com/prysmaticlabs/prysm into…

4881713

… webbackendrevertDeleteRPC

Merge branch 'webbackendrevertDeleteRPC' of https://github.com/ahadda…

6563ef6

…5/prysm into webbackendrevertDeleteRPC

ahadda5 and others added 6 commits March 31, 2021 21:28

unrelated files

06c0229

unrelated files

440b57e

unrelatedfiles restored

1b96cbe

revert unrelated files

bb1a358

changed the last ss

66ad0f9

Merge branch 'develop' into webbackendrevertDeleteRPC

6d6c410

ahadda5 and others added 2 commits April 1, 2021 17:56

restore imports

3f7e724

Merge branch 'develop' into webbackendrevertDeleteRPC

b9eb9e4

rkapka approved these changes Apr 2, 2021

View reviewed changes

rkapka merged commit 8784390 into prysmaticlabs:develop Apr 2, 2021

ahadda5 deleted the webbackendrevertDeleteRPC branch April 4, 2021 06:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web Backend Revert delete rpc #8681

Web Backend Revert delete rpc #8681

ahadda5 commented Mar 28, 2021 •

edited

Loading

rkapka left a comment

rkapka Mar 29, 2021

ahadda5 Mar 29, 2021

rkapka Mar 29, 2021

rkapka Mar 29, 2021

ahadda5 Mar 29, 2021

rkapka Mar 29, 2021

ahadda5 Mar 29, 2021

rkapka Mar 29, 2021 •

edited

Loading

ahadda5 commented Mar 29, 2021

rkapka commented Mar 29, 2021 •

edited

Loading

ahadda5 commented Mar 29, 2021

rkapka commented Mar 29, 2021

rauljordan Mar 30, 2021

ahadda5 Mar 30, 2021

rkapka Mar 31, 2021

rauljordan left a comment

rauljordan Mar 30, 2021

codecov bot commented Mar 31, 2021 •

edited

Loading

rauljordan commented Mar 31, 2021

Web Backend Revert delete rpc #8681

Web Backend Revert delete rpc #8681

Conversation

ahadda5 commented Mar 28, 2021 • edited Loading

rkapka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

ahadda5 commented Mar 29, 2021

rkapka commented Mar 29, 2021 • edited Loading

ahadda5 commented Mar 29, 2021

rkapka commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauljordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 31, 2021 • edited Loading

Codecov Report

rauljordan commented Mar 31, 2021

ahadda5 commented Mar 28, 2021 •

edited

Loading

rkapka Mar 29, 2021 •

edited

Loading

rkapka commented Mar 29, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading