Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[NWC] Add an encryption tag to negotiate upgrading to NIP44. #1780
base: master
Are you sure you want to change the base?
[NWC] Add an encryption tag to negotiate upgrading to NIP44. #1780
Changes from all commits
9c9b0b4
2f32650
89b77dd
e1fe193
13f9967
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be used? If yes, how will this error be encrypted? 😄
I think it's better to not respond to such requests to reduce complexity...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the encryption point! I'd kind of lost the fact that errors also need to be encrypted. Thinking through this a bit, I'm not sure I love not responding to these events because that's not very debuggable on the client app side and will leave client apps hanging until they time out, so it's not a great ux either. We can't encrypt the error using the client's encryption type because by definition in this case the wallet service doesn't support it.
I think the best behavior here is actually to just encrypt the error content using the wallet service's default encryption scheme. Of course the client won't be able to decrypt it, but at least they'll get a decryption error while debugging, which will tip them in the right direction. There may even be some cases where it was actually just the client's encryption tag which was wrong, and so they will see the error message. For example:
nip44
ornip44v2
instead ofnip44_v2
Does that make sense to you? If so, I can add some clarification to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, building a lot of extra logic to try to catch all the cases where a developer doesn't implement the spec properly is a slippery slope and makes more complicated code to account for all these cases as Adithya mentioned. But it's a good point that developers who cannot see the wallet service logs will find it harder to debug. In Alby Hub the developer can see the logs, but this won't be the case for services where the developer is not the owner of the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about this more, I still feel pretty strongly that the debugging experience would be pretty painful if servers just ignore these messages entirely. As a client app developer, it's pretty likely that I didn't write the software for my wallet and that I'm not running it locally. It feels like a pretty easy foot-gun to write
nip44
instead ofnip44_v2
and spend the next several hours confused why your app doesn't work. IMO being defensive here is just good devX.I do understand your point about not trying to catch every case where a developer doesn't implement the spec correctly, but in this case, even doing nothing is a choice in special handling. Even then you have already looked at the message, seen an invalid encryption scheme, and then decided to do nothing instead of returning an error. I don't think either case is really much extra handling tbh.