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

Metadata/locked transfer #6880

Merged
merged 10 commits into from
Mar 29, 2021
Merged

Metadata/locked transfer #6880

merged 10 commits into from
Mar 29, 2021

Conversation

fredo
Copy link
Contributor

@fredo fredo commented Mar 15, 2021

Description

Metadata will be fetched from PFS and then forwarded to the SendLockedTransfer Event.
The metadata for the complete route will be stored in the message in the Metadata field as well as recipient_metadata will be set.

@auto-assign auto-assign bot requested a review from konradkonrad March 15, 2021 16:21
@fredo fredo force-pushed the metadata/locked-transfer branch from 60f5974 to 1d3a123 Compare March 17, 2021 11:25
@konradkonrad konradkonrad removed their request for review March 17, 2021 17:54
@ezdac ezdac self-requested a review March 18, 2021 16:06
ezdac
ezdac suggested changes Mar 19, 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.

Looks good! I think most comments I had were actually for my code-contributions :)
You will get some conflicts in the transport (e.g. multicast_raw was renamed etc.) later on, when you rebase on the merged #6881

raiden/transfer/routes.py Outdated Show resolved Hide resolved
raiden/messages/metadata.py Outdated
return keccak(rlp.encode(self.route))
return keccak(self._rlp_serialize())

def _rlp_serialize(self) -> bytes:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also get the triple check from @ulope for this.
This hash will only be used to verify the message (part of additional_hash in the contract).
primitive_dict_to_nested_lists( ) constructs deterministically ordered nested lists of key/value tuples, which is the recommended way in rlp to represent dictionaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Input from @ulope : just use canonical json seriliasation, package already present because of synapse

Copy link
Contributor

Choose a reason for hiding this comment

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

This has been implemented now - the signature will not be compatible with older messages containing RouteMetadata

raiden/transfer/state.py Outdated Show resolved Hide resolved
raiden/utils/typing.py Show resolved Hide resolved
raiden/routing.py Outdated Show resolved Hide resolved
raiden/routing.py Show resolved Hide resolved
raiden/tests/unit/serialized_messages/Delivered.json Outdated Show resolved Hide resolved
raiden/tests/utils/transfer.py Outdated Show resolved Hide resolved
raiden/tests/utils/transfer.py Outdated Show resolved Hide resolved
raiden/transfer/channel.py Outdated Show resolved Hide resolved
@fredo fredo force-pushed the metadata/locked-transfer branch 2 times, most recently from f240ff4 to c76356b Compare March 22, 2021 08:23
ezdac
ezdac suggested changes Mar 22, 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.

Found one location where a refactoring was missed, otherwise looks good.
I would also feel better if someone else would look over the serialisation for the hash.

raiden/tests/utils/transfer.py Outdated Show resolved Hide resolved
raiden/tests/utils/transfer.py Outdated Show resolved Hide resolved
@fredo fredo force-pushed the metadata/locked-transfer branch from c76356b to 047af95 Compare March 22, 2021 09:32
@fredo fredo mentioned this pull request Mar 22, 2021
Open
16 tasks
@ezdac ezdac force-pushed the metadata/locked-transfer branch from 13d363e to 677f57e Compare March 25, 2021 14:10
ezdac
ezdac reviewed Mar 25, 2021
View reviewed changes
raiden/messages/metadata.py Outdated Show resolved Hide resolved
ezdac
ezdac reviewed Mar 25, 2021
View reviewed changes
raiden/transfer/routes.py Outdated
) -> List[RouteState]:
"""Given a selected route, returns a filtered route table that
contains only routes using the same forward channel and removes our own
address in the process.
Our address is also removed from the address metadata, if we are not the initiator
Copy link
Contributor

@ezdac ezdac Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, it would be easier for the receiver note if this would not happen, and we would include our (the senders) address-metadata all the time.
Otherwise the transport would have to inject the user-id (so that this can in turn be memorised in the state
for later creation of send-events that go back to the sender).

Copy link
Contributor

Choose a reason for hiding this comment

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

Above described behaviour has been implemented

@ezdac ezdac force-pushed the metadata/locked-transfer branch 4 times, most recently from cf97a92 to 4137f78 Compare March 26, 2021 16:06
ezdac
ezdac approved these changes Mar 29, 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.

Looks good

fredo and others added 8 commits March 29, 2021 15:02
@fredo
add recipient metadata in SendMessageEvent
3fa7a13 
add marshmallow dataclass union

use recipient metadata
@fredo
Use Metadata in Locked transfer from initiator
cc99a2e 
fix tests
make address metadata optional
consistent fixme

changes requested

fix lint

fix lint
@fredo
dont prune address metadata
3f335fc
@fredo
Use Metadata in Locked transfer from initiator
e9b0d3e 
fix tests
make address metadata optional
consistent fixme

changes requested

fix lint

fix lint
@ezdac @fredo
Change RouteMetadata hash generation to canonicaljson
396473b
@ezdac @fredo
Include node's own address in forwarded address-metadata
4869c26
@fredo
add recipient metadata in SendMessageEvent
7731c37 
add marshmallow dataclass union

use recipient metadata
@fredo
Use Metadata in Locked transfer from initiator
6f636b7 
fix tests
make address metadata optional
consistent fixme

changes requested

fix lint

fix lint
@fredo fredo force-pushed the metadata/locked-transfer branch from 6699425 to 485f3c3 Compare March 29, 2021 13:13
@fredo fredo merged commit b99220d into raiden-network:presence_less_client Mar 29, 2021
@ezdac
Copy link
Contributor

ezdac commented Apr 1, 2021

Ok, something went wrong here - the last commit 485f3c3 is missing the actual code changes and just includes the resulting serialised messages

@ezdac ezdac mentioned this pull request Apr 1, 2021
Closed
@fredo fredo mentioned this pull request Apr 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ezdac ezdac ezdac approved these changes

Assignees

@ezdac ezdac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fredo @ezdac