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-77: Implement RegisterSubnetValidatorTx #3420

Merged
merged 463 commits into from
Nov 8, 2024

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Sep 25, 2024

Why this should be merged

This PR introduces the RegisterSubnetValidatorTx defined by ACP-77.

This PR includes:

  • RegisterSubnetValidatorTx serialization definition
  • RegisterSubnetValidatorTx building and wallet support
  • (Basic) complexity calculations for RegisterSubnetValidatorTx
  • Execution logic and rules for RegisterSubnetValidatorTx
  • Example usage of RegisterSubnetValidatorTx

This PR does not include:

How this works

This PR adds the new RegisterSubnetValidatorTx type and implements all the required visitors.

How this was tested

  • Added unit tests for all new code
  • Added e2e test for new transaction type

@StephenButtolph StephenButtolph changed the title ACP-77 Implement RegisterSubnetValidatorTx ACP-77: Implement RegisterSubnetValidatorTx Sep 26, 2024
@StephenButtolph StephenButtolph added this to the v1.12.0-prerelease milestone Oct 4, 2024
@StephenButtolph StephenButtolph self-assigned this Oct 4, 2024
Base automatically changed from standardize-p-chain-visitor-order to master November 7, 2024 20:59
@StephenButtolph StephenButtolph marked this pull request as ready for review November 8, 2024 01:01
Copy link

@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.

Generally makes sense to me. Left one comment about sentence I couldn't parse.

For the TODOs that are waiting on ACP-103 numbers, it might be helpful for both reviewers of this PR and whoever fixes the TODOs to label the TODOs specifically as TODO ACP-103 or something similar.

Comment on lines 36 to 38
// to avoid recommending it as the minimum height. If the evicting, or not
// evicting, the block would violate either the maxiumum or minimum window
// size, the block will not be evicted.

Choose a reason for hiding this comment

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

This sentence doesn't seem to make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-wrote it

Copy link
Contributor

@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.

Agree with @geoff-vball suggestion for a searchable prefix on each of the TODOs added in this PR that we will want to confirm have been completed, but all makes sense to me otherwise.

@StephenButtolph
Copy link
Contributor Author

Agree with @geoff-vball suggestion for a searchable prefix on each of the TODOs added in this PR that we will want to confirm have been completed, but all makes sense to me otherwise.

I added // TODO: Before Etna as a searchable prefix. I ended up doing a kind of meta TODO rather than modifying all of the other TODOs because there are a lot of TODOs in the fee package (outside of this PR) that should also be updated if we did them all individually.

@michaelkaplan13
Copy link
Contributor

Agree with @geoff-vball suggestion for a searchable prefix on each of the TODOs added in this PR that we will want to confirm have been completed, but all makes sense to me otherwise.

I added // TODO: Before Etna as a searchable prefix. I ended up doing a kind of meta TODO rather than modifying all of the other TODOs because there are a lot of TODOs in the fee package (outside of this PR) that should also be updated if we did them all individually.

SGTM

@StephenButtolph StephenButtolph added this pull request to the merge queue Nov 8, 2024
Merged via the queue into master with commit e92cf64 Nov 8, 2024
23 checks passed
@StephenButtolph StephenButtolph deleted the implement-acp-77-register-subnet-validator-tx branch November 8, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants