-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
goat account migration #768
Conversation
return err | ||
} | ||
|
||
var op json.RawMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't do anything, json.RawMessage
is []byte
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, does it not validate that the bytes are valid JSON when doing this unmarshal? I do also like having the type wrapper, and it allows passing through to the outbound marshal step below in a way that wouldn't work if it was just []byte
.
cmd/goat/account_migrate.go
Outdated
} | ||
|
||
// display migration status | ||
// TODO: should this loop? with a delay? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah you could run this in a loop in the background to give like progress bars for blob uploads or something, but otherwise you can just do it in between steps to ensure that things finished correctly
cmd/goat/account_migrate.go
Outdated
} | ||
unsignedOp.Token = &plcToken | ||
|
||
// TODO: add some safety checks here around rotation key set intersection? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new PDS does some checks on your behalf as well. But an extra check here wouldn't hurt. The TS script I wrote doesn't do any extra checks but before we start encouraging other folks to use this script en mass we should probably make it a bit more resilient
cmd/goat/bsky_prefs.go
Outdated
return err | ||
} | ||
|
||
// TODO: does this crash with unsupported preference types? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it shouldn't
I think the client is more accepting than the server. I believe the PDS will reject requests for preferences not in the app.bsky
namespace. Which also means that users migrating off of our PDSs shouldn't run into any unsupported prefs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an indigo thing: the default de-serialization will throw an error if it encounters a $type
which isn't a Lexicon schema known at compile-time.
cmd/goat/bsky_prefs.go
Outdated
return err | ||
} | ||
|
||
// TODO: does this clobber unsupported preference types? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't! But like above, it will reject any preference types that aren't app.bsky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is partially an indigo issue: unlike in typescript, go data is deserialized in to a struct by default; re-serializing this discards and data/fields which are not in the codegen'd struct. This includes off-Lexicons fields; open union members from unknown Lexicons; and data from fields in newer versions of the Lexicon since the client code was generated.
Updated comments in the code. Going to merge now to have this basic functionality available. |
This PR adds support for account migration using the goat CLI tool two different ways:
The hope is that the migrate command should work in common cases, but the individual commands allow cleaning up in the case of partial success, and doing more complex stuff.
Missing pieces as future work:
did:web
functionality