Fix tests to not cast staked tokens amounts to int64

[sunnya97]

Summary of Bug

Currently the power reduction variable in the staking module is defaulted to 10^6.

However, if the smallest unit of a denom is very small such as 10^-18, then the number of tokens staked can be very high, making them not possible to be cast to an int64.

However, many of the staking tests incorrectly assume that number of staked tokens can be cast to int64, when this is not necessarily the case. These tests should be updated to not make this assumption. See example in this Straightedge branch here: v0.39.1...heystraightedge:powerreduction-18

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

Why did you need to fork anything? It's a var.

[sunnya97]

How do I change a var whose value is hardcoded in the code of the module?

[alexanderbez]

It's a var so it's mutable, not hard-coded.

e.g.

func init() {
  stakingtypes.PowerReduction = ...
}

[aaronc]

I would prefer to get rid of this as a global var eventually.

[alexanderbez]

You always have my support for removing globals 👍

[sunnya97]

Oh I see. Where do I put that init() function? In my app.go? That will affect the variable for when it is called from within the staking module?

Also, regardless, we do need to update the tests to not assume int64-castable

[sunnya97]

If we want to remove this as a global var, should it become an on-chain param? I can imagine some reasons why it might want to be change-able on the fly

[alexanderbez]

Yes, in app.go or anywhere else staking is related. You shouldn't need to worry about tests though.

WRT to the ideal solution, either we pass this to the keeper itself or an on-chain parameter. Can you point out some reasons why this value would need to change?

[aaronc]

I was thinking keeper argument. A param change would be pretty radical I think. Although I guess it could be useful if you wanted to make staking power more granular... right?

[sunnya97]

Yeah, the case I was particularly thinking for making it granular, is that I'm designing a Cosmos staking module that each validator has "small weights". Similar-ish the PolkaDot-style equal weight validators, but don't need operators to run multiple validators.

For example, if Validator A has a stake 54 tokens, and validator B has stake 103 tokens, I'd like to just set my power reduction to be 50, so validator A has voting power 1, and validator B has voting power 2.

And that "power reduction" variable would be dynamic based off the current distribution of stake. I have more details about the exact mechanism for my module that I can share later

[sunnya97]

You shouldn't need to worry about tests though.

Why not? I'd rather still update those tests so they don't make this incorrect assumption. I should be able to run my tests using the power reduction I'm using for my chain, which might be 10^18.

[alexanderbez]

Those tests are based in the SDK and work off of the default power reduction. You don't run another repo's tests in your repo -- that's what I meant. By all means, you should probably have tests that handle your specific power reduction value, but nothing to do with the SDK's existing tests.

Regardless, if this is an on-chain param, then that makes our lives easier.

[sunnya97]

I still think those tests should be updated. Like if someone wants to change the power reduction for their app, they would have to fix the tests (like I did).

I don't see any con to fixing the tests. The only reason I see for being the way they are is someone wanted to use require.Equal instead of require.True.

Bad code hygiene that should be fixed. It's an sdk.Int for a reason; shouldn't assume it can be cast to int64, even in tests

require.Equal(t, amt.Int64(), amt2.Int64()) vs require.True(t, amt.Equal(amt2))

[sunnya97]

I'm going to change this issue to be about fixing the tests. Will open a new issue about potentially moving PowerReduction to a param, and we can discuss that there.