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

Update Type Version Hash to use SHA-256, and details updates based on design discussion #7

Merged

Conversation

emersonknapp
Copy link

@emersonknapp emersonknapp commented Jan 11, 2023

Update for ros-infrastructure#358

From discussion today, propose rewrite of "Type Version Hash" section to reflect latest consensus. Main point - use SHA-256 instead of SHA-1. Other clarifying points on how the hash should be calculated.

… design discussion

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
…larify some language

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Author

@wjwwood @clalancette @methylDragon for review

Should the message name, including package and namespace, be part of this?
Consider a situation where you have the same data structure but two different type names, should those two instances have the same hash?
They are unique when paired with their type name, so it should be ok, but it is a bit weird, perhaps, since we're not using this type hash to check for wire compatibility between differently named types.
Finally, the resulting filled data structure must be represented in a platform-independent format, rather than running the hash function on the in-memory native type representation.
Copy link
Author

Choose a reason for hiding this comment

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

I'm a bit caught up on this part. Not sure how I should represent the TypeDescription for hashing. First instinct is always "dump it to YAML", but I'm not sure if that's the best approach. It doesn't have to be dictated here in the REP but the proposal spec, wherever that lives, will specify this strongly for RIHS1.

Copy link
Author

Choose a reason for hiding this comment

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

As discussed in weekly meeting, we will represent as JSON (implementation detail: emitted in C by specific libyaml configuration)

Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I left one comment that I think can be worded better, but I think this is a good improvement. @wjwwood can you merge this into the main REP please? I don't have permission.

rep-2011.rst Outdated Show resolved Hide resolved
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
@wjwwood wjwwood merged commit ae78cf9 into wjwwood:evolving_message_types_rep Jan 31, 2023
@emersonknapp emersonknapp deleted the rep2011-sha256 branch July 4, 2023 17:58
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.

3 participants