search

rocket-pool / smartnode

The CLI package for Rocket Pool smart nodes.
GNU General Public License v3.0
149 stars 112 forks source link

Addressing smartnode-issue-572 #621

Open SolezOfScience opened 3 months ago

SolezOfScience commented 3 months ago

Overview

This is change 1/2 to address smartnode-issue-572.

This change includes a key-recovery-manager class which is a refactored version of the existing recover-keys. For the time being, the old version is being kept and will be removed in a follow-up PR which migrates the remaining daemon API implementations to use the new version and support the new CLI parameters.

Failure Reason Collection

The approach collects errors as it iteratively tries to recover keys. If keys have not been attempted to be processed, you won't get a publicKey -> error association. However, you will get a general error response. Once publicKeys are able to be procured, the response objects are used.

Limiting Unexpected State

Overall, this change maintains the status quo for how the API tries to manage/limit risk of unexpected state and how it's documented. At their core, these APIs will always have risk of unexpected state given the serial nature of how keys are written by the keystore manager. Without a true atomic batch-write procedure, there will always be risk of the process leaving unfinished work. This change does not intend to address this, but is acknowledged as a potential improvement in the future should it become worthwhile.

Testing

No testing has been run on this change yet. I would prefer to setup unit testing rather than try enacting manual testing. I'll be following up on this topic as the PR receives feedback.

0xfornax commented 1 month ago

No testing has been run on this change yet. I would prefer to setup unit testing rather than try enacting manual testing. I'll be following up on this topic as the PR receives feedback.

Hey @SolezOfScience, thanks for the PR! Are you still considering unit tests?

SolezOfScience commented 1 month ago

Hey @0xfornax, sorry for the delay. A change in my professional role has left me with less bandwidth to finish this than I originally expected. The next few weeks are very busy for me between going to Pragma and ETHGlobal's hackathon and some personal life commitments. It is probably best to have the testing picked up by someone from the team.