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.

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

refactor: Replace Payload with PayloadMessage #527

Merged
merged 20 commits into from
Jun 3, 2024

Conversation

dariusc93
Copy link
Contributor

@dariusc93 dariusc93 commented May 24, 2024

What this PR does 📖

  • Renames PayloadRequest to PayloadMessage
  • Adds PayloadBuilder
  • Removes Payload, replacing it with PayloadMessage
  • Add PayloadMessage::addresses
  • Change topic names to separate the difference between this PR and previous implementation.

Which issue(s) this PR fixes 🔨

Special notes for reviewers 🗒️

  • This will be a breaking change as this affects the data being transmitted and would not have any backwards compatibility.

Additional comments 🎤

  • Payload was used originally to cut down on allocation when receiving bytes, however it acts more of a carrier and had less functionality and limitations while at that point it would of made more sense to either use protobuf for zero-copy or just allocate. Replacing it with PayloadMessage (originally PayloadRequest), which was originally designed for delivering identity payload, allows us to sign and verify the message itself, cosign (related to Implement subkey #253), and be more functional with different options such as attaching peer address when a message is sent (which would be helpful for Implement subkey #253 and misc: use common topics #389).
  • PayloadMessage::addresses is added as a means of providing the peer address(es) to allow one to connect in the event of content discovery. Related to Implement subkey #253, this will allow connecting to peers for content when the node public key differs from the identity public key. Additionally, messages are broadcast over a common topic while not directly connected to a peer, this would allow us to establish connection where possible or as needed.
  • Encoding/decoding will now be done with cbor instead of serde_json so we can reduce the size of the payload by using a binary format.

@dariusc93 dariusc93 self-assigned this May 24, 2024
@dariusc93 dariusc93 marked this pull request as ready for review May 31, 2024 00:03
Copy link
Contributor

@ashneverdawn ashneverdawn left a comment

Choose a reason for hiding this comment

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

lgtm.

You mentioned the lack backwards compatibility - are there any important migration steps? Is this only making sure none of the relay servers are outdated? Or are there other implications? I assume users won't lose any data and won't need to do anything.

@dariusc93
Copy link
Contributor Author

lgtm.

You mentioned the lack backwards compatibility - are there any important migration steps? Is this only making sure none of the relay servers are outdated? Or are there other implications? I assume users won't lose any data and won't need to do anything.

Just in term of the data being transmitted so previous commits would not be compatible with this commit hence the change in topic names so any data transmitted would not repeatedly log an error due to using an older format. There are no migration needed since this is just related to the data being transmitted over the network and not impact local data itself.

@dariusc93 dariusc93 merged commit c1e8550 into main Jun 3, 2024
2 checks passed
@dariusc93 dariusc93 deleted the refactor/payload-to-request-payload branch June 3, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants