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

fix(node): skip power scale if not collateral based #1149

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

Conversation

cryptoAtwill
Copy link
Contributor

The power scale is used to convert collateral staked into voting power for cometbft.

With federated power, the power scale is also used to convert federated power into voting power. This has caused a lot of confusion for many ppl. For example, if one sets the federated power to 1000, with the power scale division, the voting power set in cometbft is actually 1. For those who are not aware of power scale, they constantly ask how come? This is especially bad if two validators with federated power of, say, 1000 and 1. People would assume 1000 would have a higher power, but it turns out, the two validators have the same power.

This PR makes a quick fix to resolve the above issue at genesis. If the power comes from non-collateral based, power scale is set to 18, which is effectively no power scaling.

Perhaps a more in depth refactor would be replacing the Collateral struct with Power, but that would require breaking changes in too many places, perhaps this way is faster and safer.

@cryptoAtwill cryptoAtwill requested a review from a team as a code owner September 26, 2024 03:29
@cryptoAtwill cryptoAtwill changed the title feat(node): skip power scale if not collateral based fix(node): skip power scale if not collateral based Sep 26, 2024
) {
args.power_scale.unwrap_or(DEFAULT_POWER_SCALE)
} else {
TokenAmount::DECIMALS as PowerScale
Copy link
Contributor

Choose a reason for hiding this comment

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

will this be the value we put in set-federated-power? that would be great if so!

Copy link
Contributor

@gvelez17 gvelez17 left a comment

Choose a reason for hiding this comment

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

This is great, it will help our federated subnets be more robust! I cannot authoritatively review the code changes but definitely the intended result is correct!

) {
args.power_scale.unwrap_or(DEFAULT_POWER_SCALE)
} else {
TokenAmount::DECIMALS as PowerScale
Copy link
Contributor

Choose a reason for hiding this comment

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

@cryptoAtwill shouldn't this be 1? We want X federated power to translate directly into X weight, right? Having 18 here would force users to append 18 zeroes to whatever power they actually want to see in the subnet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a subtraction here:

d if d >= 0 => TokenAmount::DECIMALS.saturating_sub(d as usize) as u32,

@raulk raulk requested a review from karlem September 30, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

3 participants