Delete old keyshare if not in next_signers #999
Conversation
JesseAbram
requested review from
ameba23 and
HCastano
and removed request for
ameba23
| if kv_manager.kv().exists(&hex::encode(NETWORK_PARENT_KEY)).await? { | ||
| kv_manager.kv().delete(&hex::encode(NETWORK_PARENT_KEY)).await? | ||
| } |
There was a problem hiding this comment.
I would say it's misleading to have an is_() function which has a side effect - and especially in this case where it's deleting a key from the DB.
Since this function is only called once, I would just inline it at the call site so it's at least transparent to any reader that there are side effects in the case that the validator isn't a "proper" signer.
There was a problem hiding this comment.
that was my first instinct however since this is a simple test for functionality, I moved it out for testing.
To move it in I would have to rewrite the state of the mock_signers (breaking an earlier test......or put it in the dev chain init state.......or I could manually swap out the TSS key by accessing the kvdb directly)
which I could do, but it would like bloat the test code plus require a chain spin up and at least add 60 seconds to the test which seemed a lot worse
I could change the function name to account for the secondary effects tho like is_signer_or_delete_parent_key
There was a problem hiding this comment.
Even in the function name you're proposal it seems like you're trying to couple two different concepts. I do see what you mean about the ease of testing though.
I would try and think about a few other options here, but I'll approve to unblock you.
| if kv_manager.kv().exists(&hex::encode(NETWORK_PARENT_KEY)).await? { | ||
| kv_manager.kv().delete(&hex::encode(NETWORK_PARENT_KEY)).await? | ||
| } |
There was a problem hiding this comment.
Even in the function name you're proposal it seems like you're trying to couple two different concepts. I do see what you mean about the ease of testing though.
I would try and think about a few other options here, but I'll approve to unblock you.
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
…te-old-keyshare-if-not-in-next-signers
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
* master: Bump serde_json from 1.0.124 to 1.0.125 in the patch-dependencies group (#1007) Signing flow with derived accounts (#990) Add `network-jumpstart` command to `entropy-test-cli` (#1004) Refactor reshare (#994) Migrate circle-ci workflow to github actions (#991) Delete old keyshare if not in next_signers (#999)
Related #941