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 SetSubnetValidatorWeightTx #3421

Merged
merged 540 commits into from
Nov 12, 2024

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Sep 26, 2024

Why this should be merged

This PR introduces the SetSubnetValidatorWeightTx defined by ACP-77.

This PR includes:

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

This PR does not include:

How this works

This PR adds the new SetSubnetValidatorWeightTx 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 self-assigned this Oct 4, 2024
@StephenButtolph StephenButtolph added this to the v1.12.0-prerelease milestone Oct 4, 2024
…lement-acp-77--set-subnet-validator-weight-tx
…lement-acp-77--set-subnet-validator-weight-tx
…lement-acp-77--set-subnet-validator-weight-tx
Base automatically changed from implement-acp-77-register-subnet-validator-tx to master November 8, 2024 19:53
@ceyonur ceyonur mentioned this pull request Nov 10, 2024
2 tasks
currentTimestamp = e.state.GetTimestamp()
upgrades = e.backend.Config.UpgradeConfig
)
if !upgrades.IsEtnaActivated(currentTimestamp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super familiar with the code, but could the:

IsEtnaActivated()
SyntacticVerify()
VerifyMemoFieldLength()
CalculateFee()
VerifySpend()

flow be moved to a helper? Seems to be repeated (or at least close to repeated) for multiple tx types here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think this can be cleaned up... But I'd prefer it to be done holistically... Since this doesn't really impact correctness, I'm going to merge this PR and treat this as a followup.

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.

LGTM, just two questions. One if a few of the standard calls for tx verification could be combined in a helper, the other the same as @ceyonur: will SetSubnetValidatorWeightTxs be considered to have the same complexity whether the weight is 0 or not? I guess it would always be charged for the worst case if so?

@StephenButtolph StephenButtolph changed the base branch from master to refactor-p-chain-configs November 11, 2024 14:47
@StephenButtolph StephenButtolph changed the base branch from refactor-p-chain-configs to refactor-subnet-auth-verification November 11, 2024 23:19
Base automatically changed from refactor-subnet-auth-verification to master November 12, 2024 19:00
@StephenButtolph StephenButtolph added this pull request to the merge queue Nov 12, 2024
Merged via the queue into master with commit b505c48 Nov 12, 2024
23 checks passed
@StephenButtolph StephenButtolph deleted the implement-acp-77--set-subnet-validator-weight-tx branch November 12, 2024 19:49
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.

7 participants