Import Keystores Standard API Implementation #9924
Conversation
| @@ -131,33 +131,6 @@ func TestImport_DuplicateKeys(t *testing.T) { | |||
| assert.Equal(t, 1, len(keys)) | |||
| } | |||
|
|
|||
| // TestImport_NonImportedWallet is a regression test that ensures non-silent failure when importing to non-imported wallets | |||
There was a problem hiding this comment.
Derived wallets should be able to import keystores as they are merely wrappers over imported wallets
| @@ -311,30 +311,7 @@ func TestServer_WalletConfig(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestServer_ImportKeystores_FailedPreconditions_WrongKeymanagerKind(t *testing.T) { | |||
There was a problem hiding this comment.
Derived wallets should be able to import keystores as they are merely wrappers over imported wallets
| k := &keymanager.Keystore{} | ||
| if err := json.Unmarshal([]byte(req.Keystores[i]), k); err != nil { | ||
| return nil, status.Errorf( | ||
| codes.Internal, "Invalid keystore at index %d in request: %v", i, err, |
There was a problem hiding this comment.
wondering if it returns bad request if any keystore is malformed or if there should be returning it with error status
There was a problem hiding this comment.
The API spec only allows for returning a few error codes, 200, 401, 403, and 500 and nothing else, unfortunately
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "Could not import keystores: %v", err) | ||
| } | ||
| return ðpbservice.ImportKeystoresResponse{Statuses: statuses}, nil |
There was a problem hiding this comment.
we can also include the message with the status, but I need to ask about including the public key here too in the response...
There was a problem hiding this comment.
Let's revisit in a future PR
|
One side-question: What happens when the user abruptly stops the import in the middle, e.g. by pressing Ctrl+C? Will the keystore become corrupted? |
Probably, but that risk exists anywhere we are writing to the DB, so there is no real good way to prevent this if a user forcefully kills a process |
* begin * rem deleted code * delete keystores all tests * surface errors to user * add in changes * del * tests * slice * begin import process * add import keystores logic * unit tests for import * tests for all import keystores keymanager issues * change proto * pbs * renaming works * use proper request * pb * comment * gaz * fix up cli cmd * test * add gw * precond * tests * radek comments
This PR implements the
ImportKeystoresstandard API endpoint defined in here as a POST request. It allows for importing slashing protection, keystores with different passwords, and aligns with the specification. We already had an API endpoint namedImportKeystoresas part of the Prysm endpoints. This is only used by the web interface, and we are safe to rename it to something more-specific to Prysm.ImportAccountsto reserveImportKeystoresfor the standard APIImporterinterface that both the derived and imported keymanager implementImportKeystoresaccording to specification, ensuring it works as expected with necessary testsNote: Diff large due to protobuf regeneration.