x/gov client deposits tests are wrong

[facundomedica]

Summary

While debugging #11926 (from #9010) I realized that the entire file (might) contain tests that don't test what they say. I'll explain below and wait for comments to see if this requires fixing.

TL;DR These tests are querying for deposits supposedly after waiting for the voting period to pass. This should not be possible (deposits are not queryable after this period) and these tests require some fixing.

TestQueryDepositsWithoutInitialDeposit is supposedly waiting for the voting period to end but then goes and queries the deposits. This means that both the "wait for voting period to end" and the subsequent query are wrong. This can be proven by adding a bigger delay between the deposit and the query: we get an error saying that the deposit doesn't exist.

cosmos-sdk/x/gov/client/testutil/deposits.go

Lines 96 to 113 in 1715693

	func (s *DepositTestSuite) TestQueryDepositsWithoutInitialDeposit() {
	val := s.network.Validators[0]
	clientCtx := val.ClientCtx
	proposalID := s.proposalIDs[0]
	// deposit amount
	depositAmount := sdk.NewCoin(s.cfg.BondDenom, v1.DefaultMinDepositTokens.Add(sdk.NewInt(50))).String()
	_, err := MsgDeposit(clientCtx, val.Address.String(), proposalID, depositAmount)
	s.Require().NoError(err)
	// waiting for voting period to end
	_, err = s.network.WaitForHeight(2)
	s.Require().NoError(err)
	// query deposit
	deposit := s.queryDeposit(val, proposalID, false, "")
	s.Require().NotNil(deposit)
	s.Require().Equal(sdk.Coins(deposit.Amount).String(), depositAmount)

TestRejectedProposalDeposits is the test that was flagged as flaky in #9010. The name implies that it queries the deposits from an already rejected proposal; if that's true then we should get exactly 0 deposits. But the test code does the opposite and expects to get the deposits list.

This one (TestQueryProposalNotEnoughDeposits) is actually getting it right:

cosmos-sdk/x/gov/client/testutil/deposits.go

Lines 134 to 140 in 1715693

	// waiting for deposit period to end
	time.Sleep(20 * time.Second)
	// query proposal
	_, err = clitestutil.ExecTestCLICmd(clientCtx, cmd, args)
	s.Require().Error(err)
	s.Require().Contains(err.Error(), fmt.Sprintf("proposal %s doesn't exist", proposalID))

cc: @AmauryM

---

For Admin Use

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

[facundomedica]

After talking with @alexanderbez I'm sure that deposits always get deleted once the proposal ends.

cosmos-sdk/x/gov/abci.go

Lines 52 to 56 in 1715693

	if burnDeposits {
	keeper.DeleteAndBurnDeposits(ctx, proposal.Id)
	} else {
	keeper.RefundAndDeleteDeposits(ctx, proposal.Id)
	}

So I'll open a new PR removing the bad tests and hopefully adding a new one