cleanup: remove invite code from client db #4326
Conversation
joschisan
force-pushed
the
rm_invite_code
branch
from
bd58ca4 to
11bf07b
Compare
|
Does this require a client migration? We'll be leaving behind a key in the database that may represent something else in the future. |
|
Now that #3417 is closed, we should be able to write this migration? |
fedimint-client/src/db.rs
Outdated
| @@ -19,7 19,7 @@ pub enum DbKeyPrefix { | |||
| ChronologicalOperationLog = 0x2d, | |||
| CommonApiVersionCache = 0x2e, | |||
| ClientConfig = 0x2f, | |||
| ClientInviteCode = 0x30, | |||
| ClientInviteCode = 0x30, // Unused, we can get an invite code from the client config | |||
There was a problem hiding this comment.
As long as this stays here future developers will know to clean out remnant data before re-using, but ideally we'd add a migration that removes all entries with that prefix, then we can also remove this enum variant.
There was a problem hiding this comment.
@joschisan what are your thoughts about this?
There was a problem hiding this comment.
I added a comment about cleaning out remnants before reusing and would not do it for now as it would need a new migration hook for non-module db migration and maybe we get a chance in the future to toss it out with less effort, should we ever make a breaking change to the client for some unrelated reason for example.
fedimint-cli/src/lib.rs
Outdated
| let invite_code = client_config | ||
| .global | ||
| .api_endpoints | ||
| .get(&peer) | ||
| .map(|peer_url| { | ||
| InviteCode::new(peer_url.url.clone(), peer, client_config.federation_id()) | ||
| }) | ||
| .ok_or_cli_msg(CliErrorKind::GeneralFailure, "peer not found")?; |
joschisan
force-pushed
the
rm_invite_code
branch
3 times, most recently
from
09d5e1b to
c6fb0bd
Compare
joschisan
force-pushed
the
rm_invite_code
branch
3 times, most recently
from
8ebf37e to
f183a63
Compare
joschisan
force-pushed
the
rm_invite_code
branch
from
f183a63 to
cd9b5a2
Compare
| @@ -35,7 35,7 @@ pub enum DbKeyPrefix { | |||
| ChronologicalOperationLog = 0x2d, | |||
| CommonApiVersionCache = 0x2e, | |||
| ClientConfig = 0x2f, | |||
| ClientInviteCode = 0x30, | |||
| ClientInviteCode = 0x30, // Unused; clean out remnant data before re-using! | |||
There was a problem hiding this comment.
We don't need a migration. We can just always delete this key on module start, and over time it will get deleted from all clients and rotated out of existence.
There was a problem hiding this comment.
I considered this, but we could never be sure could we?
There was a problem hiding this comment.
We don't need a migration. We can just always delete this key on module start, and over time it will get deleted from all clients and rotated out of existence.
This PR has an example of how to do this https://github.com/fedimint/fedimint/pull/4427/files#diff-d754f9b1f77285e47d11bdecd4e3df652f846e5cedd12ded095ae84816a64cc2
There was a problem hiding this comment.
I considered this, but we could never be sure could we?
I mean - we would still keep ClientInviteCode variant in the code with annotation not to use it, but instead of a note to the future self: "clean out remnant data before re-using!" we would just do it eagerly.
After long enough time for all practical purposes we'd be sure no supported client can possibly still have it there.
I doubt it is going to matter in practice, but conceptually it's strictly better approach.
There was a problem hiding this comment.
I doubt it is going to matter in practice, but conceptually it's strictly better approach.
I agree with both, would however prefer to get this merged now.
There was a problem hiding this comment.
Deleting unconditionally is a hack imo and DB migrations are basically free to add.
If we eventually remove old migrations the client can refuse to start if a too old DB is encountered, but that only works if all DB migrations are modeled properly.
We can generate invite codes from the client config, the separate invite code in the db is a remnant from a time when we had to request an invite code from the guardian.