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

[ACP-99] Emit Justification Info #188

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cam-schultz
Copy link
Contributor

  • Adds information needed to construct ACP-118 justifications for ACP-77 ICM message types to be signed by P-Chain validators. See Emit justification info ava-labs/icm-contracts#741 for expanded context.
  • Adds a missing NatSpec comment
  • Formats Solidity code blocks using the stable Foundry v1.0.0 release

Comment on lines +126 to +127
* This is used along with the subnetID as the ACP-118 justification for
* signature requests from P-Chain validators over a L1ValidatorRegistrationMessage
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This is used along with the subnetID as the ACP-118 justification for
* signature requests from P-Chain validators over a L1ValidatorRegistrationMessage
* This is used along with the subnetID as the ACP-118 justification in
* signature requests to P-Chain validators over a L1ValidatorRegistrationMessage

It took me a few tries to parse this sentence. I think this is slightly more accurate, but feel free to disagree.

event InitiatedValidatorRegistration(
bytes32 indexed validationID,
bytes20 indexed nodeID,
bytes32 registrationMessageID,
uint64 registrationExpiry,
uint64 weight
uint64 weight,
bytes registerL1ValidatorMessage
Copy link
Contributor

Choose a reason for hiding this comment

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

These bytes are already emitted by the precompile as part of the sendWarpMessage event here, right? Is there a significant UX improvement to also emitting them here?

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.

2 participants