Skip to content
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

Add groupkey in init msg #62

Merged
merged 3 commits into from
Feb 20, 2023
Merged

Add groupkey in init msg #62

merged 3 commits into from
Feb 20, 2023

Conversation

Powersource
Copy link
Collaborator

for #40

@Powersource Powersource requested review from staltz and mixmix February 16, 2023 14:26
@Powersource Powersource mentioned this pull request Feb 16, 2023
6 tasks
package.json Outdated Show resolved Hide resolved
findOrCreateGroupFeed(null, function gotGroupFeed(err, groupFeed) {
// prettier-ignore
if (err) return cb(clarify(err, 'Failed to find or create group feed when creating a group'))

const secret = secretKeyFromPurpose(groupFeed.purpose)
const secret = secretKeyFromString(groupFeed.purpose)
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal, but a small suggestion:

Suggested change
const secret = secretKeyFromString(groupFeed.purpose)
const groupKey = secretKeyFromString(groupFeed.purpose)

We seem to be going back and forth with this naming, "secret" or "group key", but we could try to be at least internally consistent (all namings of this thing in ssb-tribes2 are the same).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Problem here is the spec calls it groupKey and the tribes2 api says secret. I think the proper way for fixing is a separate issue/pr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this pr is just for conforming to the spec

Copy link
Member

Choose a reason for hiding this comment

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

Okay!

groupInitMsg,
groupKey: secret.toBuffer(),
}),
secret: secret.toBuffer(),
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, could name this groupKey

)

ssb.box2.addGroupInfo(data.id, {
key: data.secret,
Copy link
Member

Choose a reason for hiding this comment

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

Case in point: the key is the secret is the key is the secret.

@staltz
Copy link
Member

staltz commented Feb 16, 2023

Tests need to be updated, they are failing just because the new field groupKey was unexpected.

@Powersource Powersource requested a review from staltz February 17, 2023 13:32
Copy link
Member

@staltz staltz left a comment

Choose a reason for hiding this comment

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

LGTM!

@Powersource Powersource merged commit 24a169c into master Feb 20, 2023
@Powersource Powersource deleted the groupkey-in-init branch February 20, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants