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

NIP-88, Polls on Nostr #1346

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Conversation

abhay-raizada
Copy link
Contributor

@abhay-raizada abhay-raizada commented Jul 5, 2024

@abhay-raizada abhay-raizada force-pushed the nostr-polls branch 2 times, most recently from b78802e to 76bc670 Compare July 5, 2024 02:54
118.md Outdated

### Poll Content

The poll event is defined as a `kind:1068` event. Major tags in the poll event are:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think defining the new kind is a good point.

118.md Outdated Show resolved Hide resolved
The poll event is defined as a `kind:1068` event. Major tags in the poll event are:

- **label**: The accompanying text of the poll.
- **option**: The option tags contain an OptionId(any alphanumeric) field, followed by an option label field.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why you need the OptionId. The label can be the ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if there are two labels with the same name? i think this is an easy way to get past that. I don't like the idea of tallying same labels together.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like a label with the same name but different meaning is not a practical option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @AsaiToshiya. If clients are only showing the name and not the option ID, the same name should mean the same vote. Not having an ID removes the need to disambiguate names when they duplicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree, the client is not doing anything in this special case.

Its the poll creator that is just having fun, maybe they want to see how the option order affects the poll results, which they should be allowed to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with abhay-raizada. I might make a poll that says: Who will you vote for? 1) Trump, 2) Trump, 3) Trump
and it should collect the three answers separately.

118.md Outdated Show resolved Hide resolved
118.md Outdated Show resolved Hide resolved
Copy link
Member

@staab staab left a comment

Choose a reason for hiding this comment

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

This is the best poll nip so far. A few minor changes, but it's the right level of dumb simplicity.

118.md Outdated Show resolved Hide resolved
118.md Outdated Show resolved Hide resolved
118.md Outdated Show resolved Hide resolved
@fiatjaf fiatjaf merged commit 4bf0c01 into nostr-protocol:master Sep 17, 2024
fiatjaf added a commit that referenced this pull request Sep 17, 2024
@fiatjaf fiatjaf changed the title NIP-118, Polls on Nostr NIP-88, Polls on Nostr Sep 17, 2024
@alexgleason
Copy link
Member

Does this have 2 implementations? I was just about to comment on it:

  • Why is response not a parameterized replaceable event?
  • Why not use the tag index for the vote instead of making up random letters?

Not using a replaceable event for the response is especially bad if the response is meant to be edited.

@alexgleason
Copy link
Member

I will actually implement polls. I have a UI for it and could do it in a couple hours. But counting votes is going to be such a pain because we don't use existing replaceable semantics.

@fiatjaf
Copy link
Member

fiatjaf commented Sep 17, 2024

  • Why is response not a parameterized replaceable event?
  • Why not use the tag index for the vote instead of making up random letters?

I asked exactly these two questions myself, but I decided it wasn't a big deal and not worth changing the way Pollerama has been doing for months and is quite good already.

We could definitely change it if @abhay-raizada agrees.

@abhay-raizada
Copy link
Contributor Author

abhay-raizada commented Sep 17, 2024

Why is response not a parameterized replaceable event?

It will cause issues with expiration, this way we can discard any responses after an expiration time. (NIP-40). Although someone could just send in lower created_at values. But at-least this method keeps open the scope for expiration, where as there's no scope with parameterized replaceable events.

"pubkey": "dee45a23c4f1d93f3a2043650c5081e4ac14a778e0acbef03de3768e4f81ac7b",
"sig": "7fa93bf3c430eaef784b0dacc217d3cd5eff1c520e7ef5d961381bc0f014dde6286618048d924808e54d1be03f2f2c2f0f8b5c9c2082a4480caf45a565ca9797",
"tags": [
["label", "Pineapple on Pizza"],

Choose a reason for hiding this comment

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

implemented NIP-88 on getwired.app
super easy to implement and works well. don't really see the point of the label tag on the poll event, but other than that no complaints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's an artifact from before the label was moved to the content field. Will update

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.

8 participants