chore! add clawback migration (prop 826)

[sainoe]

Description

Closes: #2884

---

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, client, or state breaking change (i.e., requires minor or major version bump)
• Targeted the correct branch (see PR Targeting)
• Provided a link to the relevant issue or specification
• Followed the guidelines for building SDK modules
• Included the necessary unit and integration tests
• Added a changelog entry in .changelog (for details, see contributing guidelines)
• 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

[MSalopek]

LGTM!

Left some nits and questions

[MSalopek]

Can you use an actual address?

[MSalopek]

Can you explain why this needs to be vestAccount.BaseAccount and not just the vestAccount?

[sainoe]

Mainly bc vestAccount is a ContinuousVestingAccount type which isn't required after migration, since there will only remain unlocked and vested coins in the account.
Therefore, its embedded BaseAccount field that holds the basic account value is sufficient.

The other option would have required updating the vesting account's original amount and end time, since it's lazy and calculates the vesting/vested coins according to the current block time rather than storing it.

[sainoe]

That's already in main.

[sainoe]

This func is already merged into main.

[mpoke]

Avoid panicking in the upgrade handler. Just log an error. Worst case scenario, we need to do another upgrade to do the migration again. But if the upgrade fails, it's much harder to deal with it.

[mpoke]

ditto: avoid panicking

[mpoke]

ditto: avoid panicking

[mpoke]

ditto: avoid panicking

[mpoke]

Take it outside the if condition and also use it in UndelegateCoinsFromModuleToAccount.

[mpoke]

"GetVestingCoins returns the total number of vesting coins. If no coins are vesting, nil is returned." If by the time the upgrade happens there are no coints vesting, this line will fail.

[sainoe]

Good catch.

[mpoke]

IIUC, this check that the CP account exists. Is that right?

[sainoe]

Not returning an error here shouldn't affect the states and prevent breaking the upgrade test of the git workflow.

[mpoke]

If GetVestingCoins returns nil, this will still fail as it will try to call Find on a nil pointer.

[sainoe]

Actually, GetVestingCoins never returns nil but an empty list of coins sdk.Coins{}.
Otherwise, this test would have failed.

I refactored it to remove any ambiguity.

[mpoke]

Great work @sainoe