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

fix: remove trailing commas from keystore json #2200

Merged
merged 9 commits into from
Dec 1, 2023
Merged

fix: remove trailing commas from keystore json #2200

merged 9 commits into from
Dec 1, 2023

Conversation

weboko
Copy link
Contributor

@weboko weboko commented Nov 8, 2023 •
edited
Loading

Description

In this PR I remove trailing commas from Keystore json.

How to test

Tests essentially do not change as it is not a breaking change from nwaku standpoint.

Issue

closes waku-org/js-rln#80

weboko
weboko commented Nov 8, 2023
View reviewed changes
waku/waku_keystore/keyfile.nim Outdated
json.add("id", %($u))
if VersionInKeyfile:
json.add("version", %version)
let json = %* (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dunno if it the best way to address it 🤷‍♂️

@weboko weboko mentioned this pull request Nov 8, 2023
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 8, 2023 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2200

Built from b6fa913

@alrevuelta alrevuelta requested a review from rymnc November 9, 2023 08:05
@alrevuelta
Copy link
Contributor

Can @rymnc have a look?

rymnc
rymnc reviewed Nov 9, 2023
View reviewed changes
Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

will look at an alternative in some time!

@weboko
Copy link
Contributor Author

weboko commented Nov 10, 2023

@rymnc what are alternatives you are looking at?

rymnc
rymnc approved these changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM, if you can get checks to pass!

@weboko
Copy link
Contributor Author

weboko commented Nov 20, 2023

@alrevuelta any comments from you?

@alrevuelta
Copy link
Contributor

@alrevuelta any comments from you?

No strong opinion as long as it's backward compatible. Delegating on @rymnc since its the code owner.

In this PR I remove trailing commas from Keystore json.

Might be worth adding why doing so. Not very clear to me.

@weboko
Copy link
Contributor Author

weboko commented Nov 21, 2023

Here is explanation ser - waku-org/js-rln#80 (comment)
If all good from your side - please, approve

@alrevuelta
Copy link
Contributor

alrevuelta commented Nov 21, 2023 •
edited
Loading

Here is explanation ser - waku-org/js-rln#80 (comment)
So it seems that

Keystore generated by nwaku is not a valid JSON.

Is that the case @rymnc. ?Will be break backward compatibility with old keystores?

@alrevuelta alrevuelta self-requested a review November 21, 2023 09:26
alrevuelta
alrevuelta approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

Approving, ensure CI passes and existing keystores are compatible with this new changes.

@Ivansete-status
Copy link
Collaborator

Ah, unfortunately, the tests are failing:
image

Ivansete-status
Ivansete-status approved these changes Nov 30, 2023
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for it :)!
Just added a minor nitpick comment

waku/waku_keystore/keyfile.nim Outdated Show resolved Hide resolved
@weboko weboko merged commit 103d398 into master Dec 1, 2023
9 of 10 checks passed
@weboko weboko deleted the weboko/keystore-json branch December 1, 2023 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@rymnc rymnc rymnc approved these changes

@alrevuelta alrevuelta alrevuelta approved these changes

Assignees

@weboko weboko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

js-rln keystore is not compatible with nwaku
4 participants
@weboko @alrevuelta @Ivansete-status @rymnc