Skip to content

send encrypted secret #7196

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
Merged
netcriptus merged 1 commit into from
Jul 13, 2021
Merged

send encrypted secret #7196

netcriptus merged 1 commit into from
Jul 13, 2021

Conversation

@netcriptus
Copy link
Contributor

@netcriptus netcriptus commented Jun 29, 2021

Allow encrypted secret to be sent on LockedTransfer's metadata

Fixes: #7191

@auto-assign auto-assign bot requested a review from fredo June 29, 2021 15:21
netcriptus
netcriptus commented Jun 29, 2021
View reviewed changes
istankovic
istankovic reviewed Jun 29, 2021
View reviewed changes
istankovic
istankovic reviewed Jun 29, 2021
View reviewed changes
fredo
fredo suggested changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@fredo fredo left a comment

Choose a reason for hiding this comment

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

LGTM in general, good job!

Once you have implemented the encryption with the correct public key we are good to go.

2 things to take care/notice:

  1. Somewhere deep in my memory I remember that it was not allowed to use an empty (b'0') bytestring as the secret. We should ask some Raiden OGs in the stand up.
  2. Some tests that encryption/decryption works would be good.

@netcriptus netcriptus force-pushed the feature/send-encrypted-secret-7191 branch from d11e81b to bcf1c55 Compare July 1, 2021 15:33
@netcriptus netcriptus requested a review from fredo July 1, 2021 15:42
fredo
fredo reviewed Jul 1, 2021
View reviewed changes
fredo
fredo reviewed Jul 1, 2021
View reviewed changes
@netcriptus netcriptus force-pushed the feature/send-encrypted-secret-7191 branch from 89b499a to 82ab477 Compare July 2, 2021 13:30
istankovic
istankovic reviewed Jul 5, 2021
View reviewed changes
istankovic
istankovic reviewed Jul 5, 2021
View reviewed changes
@netcriptus netcriptus force-pushed the feature/send-encrypted-secret-7191 branch 2 times, most recently from 619deb1 to d5e5de0 Compare July 7, 2021 13:20
@istankovic
Copy link
Contributor

istankovic commented Jul 8, 2021

@netcriptus nitpick: it would be nice to squash some of the changes (e.g. the pdb stuff, hex encode fix) here and split some others
(like the addition of dependencies in the "refactor tests" commit 26012b3).

@netcriptus
Copy link
Contributor Author

netcriptus commented Jul 8, 2021

@netcriptus nitpick: it would be nice to squash some of the changes (e.g. the pdb stuff, hex encode fix) here and split some others
(like the addition of dependencies in the "refactor tests" commit 26012b3).

Will most definitely do it, but sadly I'm not done with this yet. I'm working on more tests. I can promise the squash will come later.

ezdac
ezdac suggested changes Jul 8, 2021
View reviewed changes
Copy link
Contributor

@ezdac ezdac left a comment

Choose a reason for hiding this comment

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

Some things that need to be addressed.
Also we have to sync with the tsRaiden team again - (the PR could still be merged first)

@netcriptus netcriptus force-pushed the feature/send-encrypted-secret-7191 branch 3 times, most recently from 682dbbe to b1aba04 Compare July 12, 2021 14:12
@netcriptus netcriptus requested review from ezdac and fredo July 12, 2021 14:15
@netcriptus netcriptus force-pushed the feature/send-encrypted-secret-7191 branch 2 times, most recently from 6454805 to 5bcc4fa Compare July 12, 2021 15:22
fredo
fredo approved these changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@fredo fredo left a comment

Choose a reason for hiding this comment

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

Looks good to me! thanks

ezdac
ezdac suggested changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@ezdac ezdac left a comment

Choose a reason for hiding this comment

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

I discovered a flaw in the serialize-missing mechanism. I'd still rather fix the underlying issue, but possibly we can do that at a later time and merge the PR?
Still, let's discuss briefly in private before merging

@netcriptus netcriptus force-pushed the feature/send-encrypted-secret-7191 branch from 5bcc4fa to 2d13a7f Compare July 13, 2021 08:57
@netcriptus netcriptus force-pushed the feature/send-encrypted-secret-7191 branch from 2d13a7f to 0098ff9 Compare July 13, 2021 09:02
@netcriptus netcriptus merged commit 704698d into raiden-network:develop Jul 13, 2021
@netcriptus netcriptus deleted the feature/send-encrypted-secret-7191 branch July 13, 2021 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@istankovic istankovic istankovic left review comments

@fredo fredo fredo approved these changes

@ezdac ezdac ezdac approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Initiator sends encrypted secret in metadata

4 participants

@netcriptus @istankovic @fredo @ezdac