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 stringer to warp types #2712

Merged
merged 4 commits into from
Feb 10, 2024
Merged

Add stringer to warp types #2712

merged 4 commits into from
Feb 10, 2024

Conversation

aaronbuchwald
Copy link
Collaborator

Why this should be merged

This PR implements fmt.Stringer on each of the warp message/signature types including Message, UnsignedMessage, BitSetSignature, and the Signature interface.

How this works

This adds a String() function to each of the interfaces following the <type>(key1 = val1, key2 = val2, ...) string format from the consensus instance stringer functions (with the exception of Message which omits the keys since both the UnsignedMessage and Signature already include their own types).

How this was tested

Tested with one-off unit tests to see the output (not worth including in source imo):

UnsignedMessage(NetworkID = 10, SourceChainID = PsaHUQPC3dkNPNvSbteyEjC9jrg8UZfMbBadDM9QhSrd3pvAT, Payload = 7061796c6f6164)
BitSetSignature(Signers = 0000000000000000, Signature = 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000)
WarpMessage(UnsignedMessage(NetworkID = 10, SourceChainID = TtF4d2QWbk5vzQGTEPrN48x6vwgAoAmKQ9cbp79inpQmcRKES, Payload = ), BitSetSignature(Signers = , Signature = 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000))

Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

All of the other functions are defined on the pointers... I think these should be too. Aside from that lgtm

vms/platformvm/warp/unsigned_message.go Outdated Show resolved Hide resolved
vms/platformvm/warp/signature.go Outdated Show resolved Hide resolved
vms/platformvm/warp/message.go Outdated Show resolved Hide resolved
@StephenButtolph StephenButtolph added this to the v1.11.0 milestone Feb 7, 2024
@StephenButtolph StephenButtolph added the monitoring This primarily focuses on logs, metrics, and/or tracing label Feb 7, 2024
@aaronbuchwald
Copy link
Collaborator Author

All of the other functions are defined on the pointers... I think these should be too. Aside from that lgtm

This was intentional since the string function doesn't modify the values, so there's no need for them to take a pointer receiver. Also, the Message type has an UnsignedMessage value field instead of a pointer, so the alternative was to take a reference to that in its String() function, which is fine, I just slightly preferred defining the function on the value.

Lmk if you still prefer changing these functions to use the pointer receiver and I'll update it and the reference in Message's String() function.

Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

This was intentional since the string function doesn't modify the values, so there's no need for them to take a pointer receiver. Also, the Message type has an UnsignedMessage value field instead of a pointer, so the alternative was to take a reference to that in its String() function, which is fine, I just slightly preferred defining the function on the value.

Lmk if you still prefer changing these functions to use the pointer receiver and I'll update it and the reference in Message's String() function.

The main 2 reasons I feel like it should be on the pointer type are:

  • Users are normally going to hold the pointer type (because we expect/return a pointer to these structs everywhere we expose them)
    • NewUnsignedMessage
    • ParseUnsignedMessage
    • NewMessage
    • ParseMessage
      All take in/return pointers ^
  • Consistency

I'd like them to be pointer receivers unless you feel strongly against it.

vms/platformvm/warp/signature.go Outdated Show resolved Hide resolved
@aaronbuchwald
Copy link
Collaborator Author

This was intentional since the string function doesn't modify the values, so there's no need for them to take a pointer receiver. Also, the Message type has an UnsignedMessage value field instead of a pointer, so the alternative was to take a reference to that in its String() function, which is fine, I just slightly preferred defining the function on the value.
Lmk if you still prefer changing these functions to use the pointer receiver and I'll update it and the reference in Message's String() function.

The main 2 reasons I feel like it should be on the pointer type are:

  • Users are normally going to hold the pointer type (because we expect/return a pointer to these structs everywhere we expose them)

    • NewUnsignedMessage
    • ParseUnsignedMessage
    • NewMessage
    • ParseMessage
      All take in/return pointers ^
  • Consistency

I'd like them to be pointer receivers unless you feel strongly against it.

Yup, just updated

@StephenButtolph StephenButtolph added this pull request to the merge queue Feb 10, 2024
@StephenButtolph StephenButtolph removed this pull request from the merge queue due to a manual request Feb 10, 2024
@StephenButtolph StephenButtolph merged commit f17ea6a into master Feb 10, 2024
17 of 18 checks passed
@StephenButtolph StephenButtolph deleted the warp-stringer branch February 10, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monitoring This primarily focuses on logs, metrics, and/or tracing
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants