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

feat: add RadioPayload trait alias and requirements #303

Merged
merged 2 commits into from Nov 3, 2023

Conversation

hopeyen
Copy link
Collaborator

@hopeyen hopeyen commented Nov 1, 2023

Description

Made a custom trait for more generic usage of the payload field for GraphcastMessage

pub trait RadioPayload:
    Message
    + ethers::types::transaction::eip712::Eip712<Error = Eip712Error>
    + Default
    + Clone
    + 'static
    + Serialize
    + async_graphql::OutputType 
{
  fn valid_outer(&self, outer: &GraphcastMessage<Self>) -> Result<&Self, MessageError>;
}

Renamed BuildMessageError to MessageError enum

Issue link (if applicable)

Part of https://github.com/graphops/engineering-meta/issues/39

Checklist

  • Are tests up-to-date with the new changes?
  • Are docs up-to-date with the new changes? (Open PR on docs repo if necessary)

@hopeyen hopeyen self-assigned this Nov 1, 2023
@hopeyen hopeyen added type:feature New or enhanced functionality size:small Small p2 Medium priority labels Nov 1, 2023
// fn validity_check(&self, _gc: GraphcastMessage<Self>, _val: Option<String>) -> Result<&Self, MessageError> {
// Ok(self)
// }
fn valid_outer(&self, outer: &GraphcastMessage<Self>) -> Result<&Self, MessageError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is something we expect all custom Radio message types to implement, how custom should/could the impl be? If we expect valid_outer to always be the same, shouldn't we enforce it on SDK side?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should require all Radios to check if the inner payload is consistent with the general Graphcast message, although I don't expect them to always be the same. For example SimpleMessage which doesn't contain fields like nonce even though it is more of an example message type. Since we don't have a restriction on the fields for the payload type, I don't think it will be easy to check the fields between the inner and outer types.
Let me know if you think we should remove valid_outer altogether; maybe there's some other way to refactor the messages so the outer type doesn't have overlapping fields (currently identifier, nonce, graph_account, etc must be in inner for the signature and they are on the outer type for interacting with gossips and generic validations

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah this makes total sense, in the current setup this is the best way then, thank you for explaining! 💯

@hopeyen hopeyen merged commit f4e5054 into dev Nov 3, 2023
16 checks passed
@hopeyen hopeyen deleted the hope/define-radio-payload-trait branch November 3, 2023 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 Medium priority size:small Small type:feature New or enhanced functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants