Skip to content
This repository was archived by the owner on Apr 17, 2023. It is now read-only.

Identity settings in profile.json #1936

Closed
wants to merge 6 commits into from
Closed

Conversation

yknl
Copy link
Contributor

@yknl yknl commented Jun 27, 2019

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.

@yknl yknl requested review from zone117x and hstove August 1, 2019 15:28
@zone117x
Copy link
Contributor

zone117x commented Aug 1, 2019

Is this no longer a WIP?

@yknl yknl changed the title [WIP] Identity settings Identity settings Aug 1, 2019
@yknl
Copy link
Contributor Author

yknl commented Aug 1, 2019

No longer WIP

Copy link
Contributor

@hstove hstove left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

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({})
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the schema:
image

@moxiegirl moxiegirl changed the title Identity settings Collection Support: Identity settings Aug 6, 2019
@moxiegirl moxiegirl changed the title Collection Support: Identity settings Collection Support: Identity settings in profile.json Aug 6, 2019
@moxiegirl moxiegirl changed the title Collection Support: Identity settings in profile.json Identity settings in profile.json Aug 6, 2019
@zone117x
Copy link
Contributor

zone117x commented Aug 8, 2019

@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?

@timstackblock
Copy link
Contributor

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 zone117x added this to the DX Q3 Sprint 1 milestone Aug 9, 2019
@yknl
Copy link
Contributor Author

yknl commented Aug 20, 2019

@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.

Copy link
Contributor

@zone117x zone117x left a 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?

@yknl
Copy link
Contributor Author

yknl commented Oct 17, 2019

Agreed, will close this PR in favour of #1940

@yknl yknl closed this Oct 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants