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

Add a plain list of services to web-client peer info structs #3142

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

sisou
Copy link
Member

@sisou sisou commented Nov 24, 2024

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@sisou sisou added the WASM label Nov 24, 2024
@sisou sisou self-assigned this Nov 24, 2024
PreGenesisTransactions,

// Catch-all to not have to panic when new services are added
Unknown,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Unknown useful info for the web client? If not, I'd prefer to simply skip these.

The from function would return an Option<PlainService> and the PlainPeerInfo could then just ignore the Nones.

Copy link
Member Author

@sisou sisou Nov 24, 2024

Choose a reason for hiding this comment

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

I added it because I wanted to avoid silent failing. If we ever add more services, but forget to add them here, noone would ever find out.

Panicking or erroring is bad for backward-compatibility, so I think 'Unknown' is the next best thing.

That's also how I found #2986, because the new topics were not hidden.

@jsdanielh jsdanielh force-pushed the soeren/web-client-peer-services branch from c7d8108 to c801676 Compare December 6, 2024 21:39
@jsdanielh jsdanielh merged commit c801676 into albatross Dec 6, 2024
8 checks passed
@jsdanielh jsdanielh deleted the soeren/web-client-peer-services branch December 6, 2024 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants