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 codec id to message formats #479

Closed
wants to merge 4 commits into from

Conversation

cam-schultz
Copy link
Contributor

@cam-schultz cam-schultz commented Aug 2, 2024

Why this should be merged

Fixes #470
Updates the message formats in StakingMessages to reflect the latest iteration of ACP-77: avalanche-foundation/ACPs#135

How this works

  • Adds codecID to all Warp message fields.
  • Combines packRegisterSubnetValidatorMessage and packValidationInfo. packValidationInfo previously did not include the codecID and typeID in the validationID calculation. packRegisterSubnetValidatorMessage packs the full message, including the codecID and typeID.

How this was tested

forge test

How is this documented

n/a

@cam-schultz cam-schultz requested a review from a team as a code owner August 2, 2024 16:54
@cam-schultz cam-schultz changed the title codec id to message formats Add codec id to message formats Aug 2, 2024
@michaelkaplan13 michaelkaplan13 requested a review from ceyonur August 2, 2024 21:54
Copy link
Contributor

@geoff-vball geoff-vball left a comment

Choose a reason for hiding this comment

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

👍

@@ -84,46 +109,53 @@ library StakingMessages {
pure
Copy link
Collaborator

Choose a reason for hiding this comment

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

@minghinmatthewlam pointed out that it's kind of odd for the pack/unpackRegisterSubnetValidatorMessage use different message types (one the request to the P-Chain, and the other the response).

To match the updated ACP-77 spec, maybe we should go with packRegisterSubnetValidtorMessage and unpackSubnetValidatorRegistrationMessage? Not opposed to adding the request/response nomenclature otherwise either if people think that's more clear.

Copy link
Collaborator

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

Generally makes sense to me. The ACP-77 spec has updated these message format and defined the actual type IDs to be used, so we'll need to update them further here to be functional.

Given this PR doesn't actually match the current spec as is, I'd lean towards including those updates here, but not opposed to doing it in a follow up either.

@cam-schultz
Copy link
Contributor Author

Generally makes sense to me. The ACP-77 spec has updated these message format and defined the actual type IDs to be used, so we'll need to update them further here to be functional.

Given this PR doesn't actually match the current spec as is, I'd lean towards including those updates here, but not opposed to doing it in a follow up either.

Given the active iteration on the spec, I've marked this PR as draft and will bring it up to date once the spec is stable.

@cam-schultz cam-schultz marked this pull request as draft August 6, 2024 15:56
Base automatically changed from staking-manager-unit-tests to staking-contract August 8, 2024 15:24
@cam-schultz
Copy link
Contributor Author

Closing in favor of #492

@michaelkaplan13 michaelkaplan13 deleted the update-staking-messages branch September 20, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants