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.

[alexanderbez]

I'm failing to see why you'd have to update the tests if you just change the var. Since you forked it, yes, you have to obviously update the tests to have it compile and pass. But if you're just mutating the global var as proposed in my original statement, you don't have to touch any tests.

[sunnya97]

Regardless of what the value is defaulted to right now, it is a bug that we try to cast it to an int64. If we ever choose to change that default value, the tests will fail. There is a simple way to fix the tests so they won't fail when the default values change, with 0 bearing on the current tests. Why wouldn't we want to do this?

If I want to fork the staking module to make slight changes, one of which might be to change the default, I'd like to minimize the diff between my version and the official, and not having to change the tests would be good.

Also, in fact, maybe we should have the tests in the official repo also test with values other than the default 10^6. To make sure there aren't other assumptive bugs like this in the actual state machine, not just the tests.

[alexanderbez]

I'm a bit lost here. If you're importing the SDK's x/staking module, you do not need to modify any tests. You're only needing to do so because you forked it. I'm suggesting you don't fork it or we go ahead and make this an on-chain param.

[sunnya97]

I'm in favor of making it an on-chain param.

But until then, we should fix the tests to try running with a high PowerReduction (such as 10^18), to make sure there aren't any places in the staking module that break with such a high PowerReduction. But we can't run said tests, because the tests themselves are broken.

[ValarDragon]

AFAICT we have two immediate issues, and one longer term one.

Immediate issues:

• Fix the test erroneously casting to int64
• Test the SDK staking module over multiple power reduction parameters (not just 10^6)

Longer term issue w/ more discussion:

• Eventually make the power reduction parameter an on-chain param.

[alessio]

This smells like it affects Launchpad as well. @ethanfrey