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

OKEnvelope can be three or four elements. #106

Closed
wants to merge 1 commit into from

Conversation

mattn
Copy link
Contributor

@mattn mattn commented Nov 5, 2023

No description provided.

Copy link

sweep-ai bot commented Nov 5, 2023

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Nov 5, 2023

Can it? I think it should be always 4. The NIP implies it's 4, then I checked Nostream, nostr-rs-relay and strfry and they all return an empty string with an ok-true message. Also https://github.com/mikedilger/gossip errors when it sees an OK message with 3 (it errored against my https://pyramid.fiatjaf.com/ relay).

Is this breaking something for you?

I think it is much simpler if we stick to 4. Less cases to check against. And maybe rephrase the NIP to clarify that it must always be 4?

@fiatjaf fiatjaf changed the title OKEnvelop can be three or four elements. OKEnvelope can be three or four elements. Nov 5, 2023
@mattn
Copy link
Contributor Author

mattn commented Nov 5, 2023

TestOKEnvelopeEncodingAndDecoding tests it.

func TestOKEnvelopeEncodingAndDecoding(t *testing.T) {
okEnvelopes := []string{
`["OK","3bf0c63fcb93463407af97a5e5ee64fa883d107ef9e558472c4eb9aaaefaaaaa",false,"error: could not connect to the database"]`,
`["OK","3bf0c63fcb93463407af97a5e5ee64fa883d107ef9e558472c4eb9aaaefaaaaa",true]`,
}

@fiatjaf fiatjaf closed this in 29646d8 Nov 5, 2023
@fiatjaf
Copy link
Collaborator

fiatjaf commented Nov 5, 2023

OK, then I'll fix that test. It was based on me reading the spec wrongly in the past. Thank you.

Also I've been running all the tests automatically before pushing for all subpackages, but forgot to run the tests on the top-level package.

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