Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update SoV struct to align with latest ACP-77 spec #3492

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

Factored out of #3388.

How this works

  1. Adds RemainingBalanceOwner
  2. Adds DeactivationOwner
  3. Clarifies that EndAccumulatedFee == 0 implies inactive
  4. Adds Compare onto SubnetOnlyValidator
  5. Passes SubnetOnlyValidators by value rather than reference so that modifying them can be done without changing the original value

How this was tested

  • Added / modified unit tests

@StephenButtolph StephenButtolph added this to the v1.11.13 milestone Oct 23, 2024
@StephenButtolph StephenButtolph self-assigned this Oct 23, 2024

// validateConstants returns true if the constants of this validator have not
// been modified.
func (v SubnetOnlyValidator) validateConstants(o SubnetOnlyValidator) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Maybe rename to something more descriptive/a question answered by a boolean (e.g. constantsAreUnmodified)?

Also, is this method intended going to be used by non-test code in subsequent PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this helper is used is later PRs

}
return vdr, err
return vdr, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) 😄

}
}

func TestSubnetOnlyValidator_validateConstants(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) As much as I like test tables, maybe it would be easier to maintain a procedural approach that modified the parameter to validateConstants field-by-field?

Copy link
Contributor

@ceyonur ceyonur left a comment

Choose a reason for hiding this comment

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

generally lgtm, just few nits that maru already mentioned.

@StephenButtolph StephenButtolph added this pull request to the merge queue Oct 24, 2024
Merged via the queue into master with commit 05295f0 Oct 24, 2024
23 checks passed
@StephenButtolph StephenButtolph deleted the update-sov-struct branch October 24, 2024 14:08
yacovm pushed a commit to yacovm/avalanchego that referenced this pull request Oct 25, 2024
yacovm pushed a commit to yacovm/avalanchego that referenced this pull request Oct 25, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants