-
Notifications
You must be signed in to change notification settings - Fork 199
Conversation
Is this no longer a WIP? |
No longer WIP |
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.
Generally looks good, and I'm glad you're adding this apart from the collections work, as it'll make it easier to review. I added some questions, as I'm still not 100% sure how it works.
export function uploadIdentitySettings( | ||
api: { gaiaHubConfig: GaiaHubConfig, gaiaHubUrl: string}, | ||
identityKeyPair: { key: string, keyID: string }, | ||
settingsData: string |
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.
Wondering why settingsData
is a string
? I would think it would be some kind of object.
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 function expects the settings data to be already serialized. Although you could argue that the function itself should perform the serialization.
@@ -455,6 +456,7 @@ export function getBlockchainIdentities(masterKeychain, identitiesToGenerate) { | |||
const identityKeyPair = deriveIdentityKeyPair(identityOwnerAddressNode) | |||
identityKeypairs.push(identityKeyPair) | |||
identityAddresses.push(identityKeyPair.address) | |||
identitySettings.push({}) |
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.
Since there is no code using these settings yet, I'm trying to reason about how this will be used. Is identitySettings
an array, where each item is an object that represents settings for the nth
identity in the keychain?
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.
@yknl Do you know what impact this has on the stale profile.json issue #1908, and bugs with multiple accounts + non-default gaiahub issue #1909 ? Code looks good, but as far as technical debt, can't tell if it's digging us deeper into those problems. Should we tag @timstackblock to run smoke tests on this branch? |
I don't think a quick smoke test would be adequate for testing issues to .json profile because we tried that before and it was broken in several places. I think a fully planned and scheduled test once we have all the pieces in place would probably be best approach. What do you guys think? |
@zone117x This PR definitely doesn't make the stale profile.json issues any better. The storage of the settings.json essentially follows the same pattern as the profile.json. We should address them in a separate effort. |
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.
I think the changes in this PR would be easier to review in context of the full collections PR #1940
Should we close this PR in favor of only having the collections one?
Agreed, will close this PR in favour of #1940 |
Adds a settings file for each identity stored on the same Gaia hub location as the profile.json. This is required for storing collection keys.