fix: (v0.45.x) regression in return value of WithdrawDelegationRewards when rewards are zero

[nddeluca]

Description

Related to #13340 and also noticed by Terra terra-money@996da0f

In v0.45.9, the WithdrawDelegationRewards returns a invalid coins when delegation rewards are zero. While this isn't state breaking, it does break state for modules depending on valid coins being returned without a zero value.

Technically an api breaking change from v0.45.9.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[alexanderbez]

I think we should do this on 0.46 too.

[nddeluca]

Would it be better to fix on main and backport for 0.46.x and 0.45.x?

[alexanderbez]

So to be clear with the PR that introduced this improvement, finalRewards is always valid due to the denom being present under all circumstances.

Now with this PR, you allow the denom to be empty and thus an invalid coin. Is this what you intend?

[alexanderbez]

Ahhh, ok to summarize:

1. v0.45.x did NOT have the if rewards.IsZero() { // ... } check so this PR reverts to the original behavior. Thank you 🙏
2. As of v0.46.2 it DOES have the if rewards.IsZero() { // ... } check, so we need to keep it there.

[nddeluca]

Correct, really what this PR solves is:

// On v0.45.8 with zero rewards we have:
amount, _ := app.DistrKeeper.WithdrawDelegationRewards(ctx, sdk.AccAddress(valAddrs[0]), valAddrs[0])
amount.IsValid() == true

// 0n v0.45.9 with zero rewards with have:
amount, _ := app.DistrKeeper.WithdrawDelegationRewards(ctx, sdk.AccAddress(valAddrs[0]), valAddrs[0])
amount.IsValid() == false

which breaks API expectations of downstream modules expecting IsValid() to return true in all cases. This can cause a panic if IsZero or IsValid isn't checked under that assumption, and the amount is passed to other coin/module methods.

For v0.46.0, v0.46.1, v0.46.2 it looks like IsValid() always returns false when rewards are zero, so the same breaking change doesn't occur. Though imo, maybe it is a little unexpected for a WithdrawDelegationRewards to return a non-valid coins value? I'm not sure if that behavior is desired?

That is, is it best practice to ensure no zero coins are included in coins such that len(coins) == 0 when coins.IsZero() == true? I guess based on the constructor sdk.NewCoins(...) removing zero coins maybe it should be? Or should modules generally always check for valid coins after method calls to modules?

[alexanderbez]

I don't see how IsValid() is true in either case?

Assume actual rewards are zero. Then prior to my PR, you'd have {denom: "", amount: 0}, which is invalid. With your reversion, it's somehow valid?

In 0.46, you always have `{denom: "", amount: 0}, which is technically also not valid -- never will be since it's a zero amount.

[nddeluca]

This passes on v0.45.8, and the goal of this PR is to pass the same assertions.

amount, err := app.DistrKeeper.WithdrawDelegationRewards(ctx, sdk.AccAddress(valAddrs[0]), valAddrs[0])
require.NoError(t, err)
require.True(t, amount.IsZero())

require.Equal(t, "", amount.String()) // "0stake" on v0.45.9
require.Equal(t, len(amount), 0) // 1 on v0.45.9
require.True(t, amount.IsValid()) // False on v0.45.9

I believe the DecCoin#TruncateDecimal call at

cosmos-sdk/x/distribution/keeper/delegation.go

Line 165 in 12eb914

finalRewards, remainder := rewards.TruncateDecimal()

drops the zero value and results in a sdk.Coins(nil) value for finalRewards, which is returned directly in v0.45.8.

Maybe that wasn't intended before or the change in v0.45.9 is outside the scope of what is considered a breaking change?

[alexanderbez]

I agree that we should fix the event regression, but I want to note that all versions <= 0.45.8 are NOT valid. I.e. amount.String() == "" is not valid, it should be 0<denom>. So my approval stands for 0.45.x, just pointing this out :)

[nddeluca]

Thank you @alexanderbez, completely agree -- this should only target v0.45.x