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

Enforce uint64max weights #650

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Enforce uint64max weights #650

merged 2 commits into from
Nov 22, 2024

Conversation

cam-schultz
Copy link
Contributor

@cam-schultz cam-schultz commented Nov 19, 2024

Why this should be merged

Enforces that validator weights cannot exceed uint64max, which is the validator weight cap enforced on the P-Chain.

How this works

Adds an explicit check in _initializeValidatorRegistration. Note that the churn tracker also enforces this bound, but an explicit check is more readable and easier to test functionally.

Also updates all variables/fields/params that represent weights to use uint64

How this was tested

New unit test

How is this documented

N/A

@@ -346,7 +346,7 @@ func InitializeNativeTokenValidatorSet(
validationIDs = append(validationIDs, subnetInfo.SubnetID.Append(uint32(i)))
}

Expect(initialValidatorCreatedEvent.Weight).Should(Equal(new(big.Int).SetUint64(nodes[0].Weight)))
Expect(initialValidatorCreatedEvent.Weight).Should(Equal(nodes[0].Weight))
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a nice side effect that these are now primitive types

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.

I love how much this code simplifies by making the go params ints!

Can we add documentation, stating that the reason we cap the total weight at a uint64 is because the p-chain does the same?

Copy link
Contributor

@bernard-avalabs bernard-avalabs left a comment

Choose a reason for hiding this comment

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

LGTM

@cam-schultz
Copy link
Contributor Author

Can we add documentation, stating that the reason we cap the total weight at a uint64 is because the p-chain does the same?

TBH, I think linking to the ACP-77 spec sufficiently documents this. I don't think it'd be helpful to explicitly document this data type here over the data type of any other field. Note that we do document how this limit affects staked value for the PoS case: https://github.com/ava-labs/teleporter/blob/main/contracts/validator-manager/README.md?plain=1#L72

@cam-schultz cam-schultz merged commit 8dc720c into main Nov 22, 2024
17 checks passed
@cam-schultz cam-schultz deleted the uint64-weights branch November 22, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants