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

Process encrypted secret by the target #7252

Merged
merged 2 commits into from
Aug 10, 2021
Merged

Process encrypted secret by the target #7252

merged 2 commits into from
Aug 10, 2021

Conversation

netcriptus
Copy link
Contributor

Process encrypted secret by the target

Fixes: #7157

@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch 7 times, most recently from 09070f4 to 858bb8e Compare July 27, 2021 15:08
@netcriptus netcriptus changed the title include and forward encrypted secret in locked transfer message Process encrypted secret by the target Jul 28, 2021
@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch 6 times, most recently from c0f30ff to 1151e56 Compare July 29, 2021 09:44
ulope
ulope reviewed Jul 29, 2021
View reviewed changes
raiden/transfer/channel.py Outdated Show resolved Hide resolved
raiden/message_handler.py Outdated Show resolved Hide resolved
raiden/message_handler.py Outdated Show resolved Hide resolved
raiden/message_handler.py Outdated Show resolved Hide resolved
raiden/message_handler.py Outdated Show resolved Hide resolved
@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch from 1151e56 to 32c55c7 Compare July 29, 2021 12:07
@netcriptus netcriptus marked this pull request as ready for review July 29, 2021 14:48
@auto-assign auto-assign bot requested a review from istankovic July 29, 2021 14:48
@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch 2 times, most recently from 4d45b86 to 08d0143 Compare August 2, 2021 13:47
ulope
ulope approved these changes Aug 2, 2021
View reviewed changes
Copy link
Collaborator

@ulope ulope left a comment

Choose a reason for hiding this comment

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

LGTM

Very nice that we could get rid of the elif dispatch boilerplate...

fredo
fredo previously requested changes Aug 2, 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.

Im just requesting changes so that it does not get merged accidentally. We figured out that decryption of the secret is currently not tested and that it should not work because of a missing targetstate because ActionTargetInit is never applied.

raiden/message_handler.py Outdated Show resolved Hide resolved
raiden/message_handler.py Outdated Show resolved Hide resolved
@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch 8 times, most recently from dd9e858 to 1f308ca Compare August 5, 2021 13:51
@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch 3 times, most recently from 3d1d85b to cf8bbe5 Compare August 6, 2021 12:47
@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch 3 times, most recently from 79a1814 to 1c7219d Compare August 6, 2021 13:51
@netcriptus netcriptus requested a review from fredo August 6, 2021 14:14
@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch from 1c7219d to f3ada0c Compare August 6, 2021 14:34
@netcriptus netcriptus force-pushed the feature/encrypted-secret-7157 branch from f3ada0c to ded77fc Compare August 6, 2021 14:40
@netcriptus netcriptus dismissed fredo’s stale review August 6, 2021 15:05

Changes implemented

@netcriptus netcriptus requested a review from ulope August 9, 2021 06:41
ulope
ulope approved these changes Aug 9, 2021
View reviewed changes
Copy link
Collaborator

@ulope ulope left a comment

Choose a reason for hiding this comment

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

This time LGTM for real ;)

Please see my comment on the test patching below.
This can be done in a follow up PR though so as not to hold this up any longer.

raiden/tests/integration/api/rest/test_rest.py
@@ -937,7 +939,10 @@ def test_channel_events_raiden(
@pytest.mark.parametrize("number_of_nodes", [3])
@pytest.mark.parametrize("channels_per_node", [CHAIN])
@pytest.mark.parametrize("enable_rest_api", [True])
def test_pending_transfers_endpoint(raiden_network: List[RaidenService], token_addresses):
@patch("raiden.message_handler.decrypt_secret", side_effect=InvalidSecret)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually use pytest's monkeypatch fixture instead of patch in the test suite (although not always...).
The advantage is that the monkeypatch fixture is integrated with pytest and also defaults to raising an exception if the patch target doesn't exist (e.g. after a refactoring) which is not the case with patch.

I think it would be nicer to put this into a fixture and / or test helper (e.g. disable_encrypted_secret) so as not having to repeat this patch boilerplate on many tests (which would need to be changed if ever the decrypt_secret function was moved or otherwise refactored).

Also this only tests the branch where the included secret is invalid, not the one where none is provided in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #7287 for this

@ulope ulope mentioned this pull request Aug 9, 2021
Merged
@netcriptus netcriptus mentioned this pull request Aug 10, 2021
Open
@netcriptus netcriptus merged commit 3730e0e into raiden-network:develop Aug 10, 2021
@netcriptus netcriptus deleted the feature/encrypted-secret-7157 branch August 10, 2021 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulope ulope ulope approved these changes

@istankovic istankovic Awaiting requested review from istankovic

@fredo fredo Awaiting requested review from fredo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Process encrypted secret by the target
3 participants
@netcriptus @ulope @fredo