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

refactor: json_annotation #11

Closed
ethicnology opened this issue Jan 27, 2023 · 3 comments
Closed

refactor: json_annotation #11

ethicnology opened this issue Jan 27, 2023 · 3 comments
Assignees

Comments

@ethicnology
Copy link
Owner

ethicnology commented Jan 27, 2023

@ryzizub proposed json_annotation in #4 and #10

This refactoring would reduce the code maintenance and solve some decoding issues.

During the code review of his PR, I noticed that some checks were removed, I tried to put them again, tests failed.
Since there are a lot of changes in this PR,I restarted from develop branch and introduced a minimalist change with json_annotation.

This is the commit to solve: aec073a

Tests are failing on these two assert since the id produced by getEventId() does not match the provided (valid) id:

    assert(this.id == id);
    assert(bip340.verify(pubkey, id, sig));

I also notice that there will be an issue with Event.from() since id is generated by getEventId() which need a late initialization that seem not compatible with this json_annotation, see LateInitializationError: Field 'id' has not been initialized.

We need to find why json_annotation is introducing a side effect on the getEventId() function which makes generates wrongs id that triggers the assertions.

We also have to find how to deal with the Event.from() and late id since this function makes really handy to create and event without generating his id and sig

@ryzizub
Copy link
Contributor

ryzizub commented Jan 28, 2023

@ethicnology regarding to your issues im have updated the branch

  1. There was breaking change that contructors where changed from positioned params to named params. I still thinks that positioned params are better, but to make this marge less breaking i changed it to positioned params for now

  2. Regarding to failing events, i found that the tests were wrong.
    Currently the asserts are not called in main branch in those two tests which failed in my branch. In my branch are asserts now called so its making it more bullet proof. Seems that the event pushed to code was also wrong, i tried 5 different events and all went ok. Maybe @ethicnology can you verify that event, i was not able to find it on my relays.

@ryzizub
Copy link
Contributor

ryzizub commented Jan 28, 2023

I updated my branch with the changes that im mentioning 73314d7

@ryzizub
Copy link
Contributor

ryzizub commented Jan 28, 2023

Changed events are here 73daaf5

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

No branches or pull requests

2 participants