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.

Sign up for GitHub

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

Jump to bottom

Re-enable checking of add-member spec #58

Merged
merged 14 commits into from
Mar 8, 2023
Merged

Re-enable checking of add-member spec #58

merged 14 commits into from
Mar 8, 2023

Conversation

Powersource
Copy link
Collaborator

Fixes #43

Powersource
Powersource commented Feb 10, 2023
View reviewed changes
index.js Outdated Show resolved Hide resolved
@Powersource Powersource mentioned this pull request Feb 10, 2023
Open
mixmix
mixmix reviewed Feb 13, 2023
View reviewed changes
index.js Outdated Show resolved Hide resolved
Powersource
Powersource commented Feb 15, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
Powersource
Powersource commented Feb 15, 2023
View reviewed changes
index.js Outdated Show resolved Hide resolved
@Powersource Powersource marked this pull request as ready for review February 15, 2023 20:27
@Powersource Powersource requested review from staltz and mixmix February 15, 2023 20:28
Powersource
Powersource commented Feb 15, 2023
View reviewed changes
index.js Outdated Show resolved Hide resolved
mixmix
mixmix reviewed Feb 16, 2023
View reviewed changes
Copy link
Member

@mixmix mixmix left a comment

Choose a reason for hiding this comment

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

Looks ready apart from waiting for that module to be installed when it's published

index.js Outdated Show resolved Hide resolved
index.js
version: 'v2',
groupKey: secret.toString('base64'),
root,
creator: rootAuthorId,
Copy link
Member

Choose a reason for hiding this comment

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

I might have missed this (creator) being added to the spec? I dislike that it's like author. Why do we need it?

creator is the root metafeed id of the creator of the group

Nope, I was involved! I think we should call it groupCreatorRootFeedId or initAuthorRootrId .. if it's not too late :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not too late! But this PR just adds the checking against the spec. Changing the field name would IMO be a different issue/pr.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@Powersource Powersource requested a review from mixmix February 16, 2023 13:19
@Powersource
Copy link
Collaborator Author

Looks ready apart from waiting for that module to be installed when it's published

i'll take this as approval now that metafeeds is updated in here

@Powersource Powersource merged commit daa179a into master Mar 8, 2023
@Powersource Powersource deleted the readd-add-member-spec-check branch March 8, 2023 09:58
@mixmix mixmix mentioned this pull request Feb 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staltz staltz Awaiting requested review from staltz

@mixmix mixmix Awaiting requested review from mixmix

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-add checking of add-member spec
2 participants
@Powersource @mixmix