Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(gov)!: use collections for Proposal state management (part 1) #16171

Merged
merged 16 commits into from
May 16, 2023
Merged

refactor(gov)!: use collections for Proposal state management (part 1) #16171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9e84217
remove set proposal method
May 16, 2023
fcf2e7e
remove GetProposalID
May 16, 2023
f5aebe8
remove GetProposalFiltered
May 16, 2023
9f22833
remove IterateProposal and GetProposals
May 16, 2023
dcfa45d
remove MarshalProposal
May 16, 2023
f705d3e
remove UnmarshalProposal
May 16, 2023
0149fa3
remove unused errors
May 16, 2023
1de9fc7
simplify proposals query logic
May 16, 2023
a9b4692
delete proposal using coll
May 16, 2023
cdbe6e0
remove GetProposal
May 16, 2023
938000a
chore: CHANGELOG.md
May 16, 2023
97a703b
chore: lint
May 16, 2023
38a13ab
fix test
May 16, 2023
c031c04
Merge branch 'main' into tip/gov/coll-proposals
testinginprod May 16, 2023
5638d52
fix lint and tests
May 16, 2023
fd11de7
Merge remote-tracking branch 'origin/tip/gov/coll-proposals' into tip…
May 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/gov) [#16164](https://github.com/cosmos/cosmos-sdk/pull/16164) Use collections for vote state management:
- Removed: types `VoteKey`, `VoteKeys`
- Removed: keeper `IterateVotes`, `IterateAllVotes`, `GetVotes`, `GetVote`, `SetVote`
* (x/gov) [#16171](https://github.com/cosmos/cosmos-sdk/pull/16171) Use collections for proposal state management (part 1):
- Removed: keeper: `GetProposal`, `UnmarshalProposal`, `MarshalProposal`, `IterateProposal`, `GetProposal`, `GetProposalFiltered`, `GetProposals`, `GetProposalID`, `SetProposalID`
- Remove: errors unused errors

* (sims) [#16155](https://github.com/cosmos/cosmos-sdk/pull/16155)
* `simulation.NewOperationMsg` now marshals the operation msg as proto bytes instead of legacy amino JSON bytes.
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/gov/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ func (s *E2ETestSuite) TestNewCmdCancelProposal() {
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
},
false, 17,
false, 1,
},
}

Expand Down Expand Up @@ -430,7 +430,7 @@ func (s *E2ETestSuite) TestNewCmdDeposit() {
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
},
false, 2,
false, 1,
},
{
"deposit on existing proposal",
Expand Down
14 changes: 7 additions & 7 deletions tests/integration/gov/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,9 @@ func TestImportExportQueues(t *testing.T) {
assert.NilError(t, err)
assert.Assert(t, votingStarted)

proposal1, err = s1.GovKeeper.GetProposal(ctx, proposalID1)
proposal1, err = s1.GovKeeper.Proposals.Get(ctx, proposalID1)
assert.NilError(t, err)
proposal2, err = s1.GovKeeper.GetProposal(ctx, proposalID2)
proposal2, err = s1.GovKeeper.Proposals.Get(ctx, proposalID2)
assert.NilError(t, err)
assert.Assert(t, proposal1.Status == v1.StatusDepositPeriod)
assert.Assert(t, proposal2.Status == v1.StatusVotingPeriod)
Expand Down Expand Up @@ -152,9 +152,9 @@ func TestImportExportQueues(t *testing.T) {
ctx2 = ctx2.WithBlockTime(ctx2.BlockHeader().Time.Add(*params.MaxDepositPeriod).Add(*params.VotingPeriod))

// Make sure that they are still in the DepositPeriod and VotingPeriod respectively
proposal1, err = s2.GovKeeper.GetProposal(ctx2, proposalID1)
proposal1, err = s2.GovKeeper.Proposals.Get(ctx2, proposalID1)
assert.NilError(t, err)
proposal2, err = s2.GovKeeper.GetProposal(ctx2, proposalID2)
proposal2, err = s2.GovKeeper.Proposals.Get(ctx2, proposalID2)
assert.NilError(t, err)
assert.Assert(t, proposal1.Status == v1.StatusDepositPeriod)
assert.Assert(t, proposal2.Status == v1.StatusVotingPeriod)
Expand All @@ -166,10 +166,10 @@ func TestImportExportQueues(t *testing.T) {
err = gov.EndBlocker(ctx2, s2.GovKeeper)
assert.NilError(t, err)

proposal1, err = s2.GovKeeper.GetProposal(ctx2, proposalID1)
assert.ErrorContains(t, err, "proposal 1 doesn't exist")
proposal1, err = s2.GovKeeper.Proposals.Get(ctx2, proposalID1)
assert.ErrorContains(t, err, "not found")

proposal2, err = s2.GovKeeper.GetProposal(ctx2, proposalID2)
proposal2, err = s2.GovKeeper.Proposals.Get(ctx2, proposalID2)
assert.NilError(t, err)
assert.Assert(t, proposal2.Status == v1.StatusRejected)
}
4 changes: 2 additions & 2 deletions tests/integration/gov/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func TestGRPCQueryTally(t *testing.T) {
func() {
proposal.Status = v1.StatusPassed
app.GovKeeper.SetProposal(ctx, proposal)
proposal, _ = app.GovKeeper.GetProposal(ctx, proposal.Id)
proposal, _ = app.GovKeeper.Proposals.Get(ctx, proposal.Id)

req = &v1.QueryTallyResultRequest{ProposalId: proposal.Id}

Expand Down Expand Up @@ -225,7 +225,7 @@ func TestLegacyGRPCQueryTally(t *testing.T) {
func() {
proposal.Status = v1.StatusPassed
app.GovKeeper.SetProposal(ctx, proposal)
proposal, _ = app.GovKeeper.GetProposal(ctx, proposal.Id)
proposal, _ = app.GovKeeper.Proposals.Get(ctx, proposal.Id)

req = &v1beta1.QueryTallyResultRequest{ProposalId: proposal.Id}

Expand Down
30 changes: 15 additions & 15 deletions tests/integration/gov/keeper/tally_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestTallyNoOneVotes(t *testing.T) {
proposal.Status = v1.StatusVotingPeriod
app.GovKeeper.SetProposal(ctx, proposal)

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -55,7 +55,7 @@ func TestTallyNoQuorum(t *testing.T) {
err = app.GovKeeper.AddVote(ctx, proposalID, addrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), "")
assert.NilError(t, err)

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, _, _ := app.GovKeeper.Tally(ctx, proposal)
assert.Assert(t, passes == false)
Expand All @@ -81,7 +81,7 @@ func TestTallyOnlyValidatorsAllYes(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[1], v1.NewNonSplitVoteOption(v1.OptionYes), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[2], v1.NewNonSplitVoteOption(v1.OptionYes), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand All @@ -108,7 +108,7 @@ func TestTallyOnlyValidators51No(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[0], v1.NewNonSplitVoteOption(v1.OptionYes), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[1], v1.NewNonSplitVoteOption(v1.OptionNo), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, _, _ := app.GovKeeper.Tally(ctx, proposal)

Expand All @@ -134,7 +134,7 @@ func TestTallyOnlyValidators51Yes(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[0], v1.NewNonSplitVoteOption(v1.OptionNo), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[1], v1.NewNonSplitVoteOption(v1.OptionYes), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -162,7 +162,7 @@ func TestTallyOnlyValidatorsVetoed(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[1], v1.NewNonSplitVoteOption(v1.OptionYes), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[2], v1.NewNonSplitVoteOption(v1.OptionNoWithVeto), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -190,7 +190,7 @@ func TestTallyOnlyValidatorsAbstainPasses(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[1], v1.NewNonSplitVoteOption(v1.OptionNo), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[2], v1.NewNonSplitVoteOption(v1.OptionYes), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -218,7 +218,7 @@ func TestTallyOnlyValidatorsAbstainFails(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[1], v1.NewNonSplitVoteOption(v1.OptionYes), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddrs[2], v1.NewNonSplitVoteOption(v1.OptionNo), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -246,7 +246,7 @@ func TestTallyOnlyValidatorsNonVoter(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddr1, v1.NewNonSplitVoteOption(v1.OptionYes), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, valAccAddr2, v1.NewNonSplitVoteOption(v1.OptionNo), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -284,7 +284,7 @@ func TestTallyDelgatorOverride(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[3], v1.NewNonSplitVoteOption(v1.OptionYes), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[4], v1.NewNonSplitVoteOption(v1.OptionNo), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -321,7 +321,7 @@ func TestTallyDelgatorInherit(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[1], v1.NewNonSplitVoteOption(v1.OptionNo), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[2], v1.NewNonSplitVoteOption(v1.OptionYes), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -363,7 +363,7 @@ func TestTallyDelgatorMultipleOverride(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[2], v1.NewNonSplitVoteOption(v1.OptionYes), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[3], v1.NewNonSplitVoteOption(v1.OptionNo), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -406,7 +406,7 @@ func TestTallyDelgatorMultipleInherit(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[1], v1.NewNonSplitVoteOption(v1.OptionNo), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[2], v1.NewNonSplitVoteOption(v1.OptionNo), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -451,7 +451,7 @@ func TestTallyJailedValidator(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[1], v1.NewNonSplitVoteOption(v1.OptionNo), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[2], v1.NewNonSplitVoteOption(v1.OptionNo), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down Expand Up @@ -486,7 +486,7 @@ func TestTallyValidatorMultipleDelegations(t *testing.T) {
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[1], v1.NewNonSplitVoteOption(v1.OptionNo), ""))
assert.NilError(t, app.GovKeeper.AddVote(ctx, proposalID, addrs[2], v1.NewNonSplitVoteOption(v1.OptionYes), ""))

proposal, ok := app.GovKeeper.GetProposal(ctx, proposalID)
proposal, ok := app.GovKeeper.Proposals.Get(ctx, proposalID)
assert.Assert(t, ok)
passes, burnDeposits, tallyResults, _ := app.GovKeeper.Tally(ctx, proposal)

Expand Down
14 changes: 7 additions & 7 deletions x/gov/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func TestTickPassedVotingPeriod(t *testing.T) {
require.True(t, activeQueue.Valid())

activeProposalID := types.GetProposalIDFromBytes(activeQueue.Value())
proposal, err := suite.GovKeeper.GetProposal(ctx, activeProposalID)
proposal, err := suite.GovKeeper.Proposals.Get(ctx, activeProposalID)
require.Nil(t, err)
require.Equal(t, v1.StatusVotingPeriod, proposal.Status)

Expand All @@ -317,7 +317,7 @@ func TestTickPassedVotingPeriod(t *testing.T) {
require.True(t, activeQueue.Valid())

activeProposalID = types.GetProposalIDFromBytes(activeQueue.Value())
proposal, err = suite.GovKeeper.GetProposal(ctx, activeProposalID)
proposal, err = suite.GovKeeper.Proposals.Get(ctx, activeProposalID)
require.Nil(t, err)
require.Equal(t, v1.StatusVotingPeriod, proposal.Status)
require.False(t, proposal.Expedited)
Expand Down Expand Up @@ -449,7 +449,7 @@ func TestEndBlockerProposalHandlerFailed(t *testing.T) {
require.True(t, eventOk)
require.Contains(t, attr[0].Value, "failed on execution")

proposal, err = suite.GovKeeper.GetProposal(ctx, proposal.Id)
proposal, err = suite.GovKeeper.Proposals.Get(ctx, proposal.Id)
require.Nil(t, err)
require.Equal(t, v1.StatusFailed, proposal.Status)
}
Expand Down Expand Up @@ -549,7 +549,7 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) {
require.True(t, activeQueue.Valid())

activeProposalID := types.GetProposalIDFromBytes(activeQueue.Value())
proposal, err := suite.GovKeeper.GetProposal(ctx, activeProposalID)
proposal, err := suite.GovKeeper.Proposals.Get(ctx, activeProposalID)
require.Nil(t, err)
require.Equal(t, v1.StatusVotingPeriod, proposal.Status)

Expand All @@ -569,7 +569,7 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) {
if tc.expeditedPasses {
require.False(t, activeQueue.Valid())

proposal, err = suite.GovKeeper.GetProposal(ctx, activeProposalID)
proposal, err = suite.GovKeeper.Proposals.Get(ctx, activeProposalID)
require.Nil(t, err)

require.Equal(t, v1.StatusPassed, proposal.Status)
Expand All @@ -593,7 +593,7 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) {
activeProposalID = types.GetProposalIDFromBytes(activeQueue.Value())
activeQueue.Close()

proposal, err = suite.GovKeeper.GetProposal(ctx, activeProposalID)
proposal, err = suite.GovKeeper.Proposals.Get(ctx, activeProposalID)
require.Nil(t, err)
require.Equal(t, v1.StatusVotingPeriod, proposal.Status)
require.False(t, proposal.Expedited)
Expand Down Expand Up @@ -639,7 +639,7 @@ func TestExpeditedProposal_PassAndConversionToRegular(t *testing.T) {
activeQueue, _ = suite.GovKeeper.ActiveProposalQueueIterator(ctx, ctx.BlockHeader().Time)
require.False(t, activeQueue.Valid())

proposal, err = suite.GovKeeper.GetProposal(ctx, activeProposalID)
proposal, err = suite.GovKeeper.Proposals.Get(ctx, activeProposalID)
require.Nil(t, err)

if tc.regularEventuallyPassing {
Expand Down
12 changes: 8 additions & 4 deletions x/gov/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

// InitGenesis - store genesis parameters
func InitGenesis(ctx sdk.Context, ak types.AccountKeeper, bk types.BankKeeper, k *keeper.Keeper, data *v1.GenesisState) {
err := k.SetProposalID(ctx, data.StartingProposalId)
err := k.ProposalID.Set(ctx, data.StartingProposalId)
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -79,13 +79,17 @@ func InitGenesis(ctx sdk.Context, ak types.AccountKeeper, bk types.BankKeeper, k

// ExportGenesis - output genesis parameters
func ExportGenesis(ctx sdk.Context, k *keeper.Keeper) (*v1.GenesisState, error) {
startingProposalID, err := k.GetProposalID(ctx)
startingProposalID, err := k.ProposalID.Peek(ctx)
if err != nil {
return nil, err
}

proposals, err := k.GetProposals(ctx)
if err != nil {
var proposals v1.Proposals
err = k.Proposals.Walk(ctx, nil, func(_ uint64, value v1.Proposal) (stop bool, err error) {
proposals = append(proposals, &value)
return false, nil
})
if err != nil && !errors.Is(err, collections.ErrInvalidIterator) {
return nil, err
}

Expand Down
2 changes: 1 addition & 1 deletion x/gov/keeper/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func setupGovKeeper(t *testing.T) (
// Gov keeper initializations

govKeeper := keeper.NewKeeper(encCfg.Codec, storeService, acctKeeper, bankKeeper, stakingKeeper, distributionKeeper, msr, types.DefaultConfig(), govAcct.String())
govKeeper.SetProposalID(ctx, 1)
require.NoError(t, govKeeper.ProposalID.Set(ctx, 1))
govRouter := v1beta1.NewRouter() // Also register legacy gov handlers to test them too.
govRouter.AddRoute(types.RouterKey, v1beta1.ProposalHandler)
govKeeper.SetLegacyRouter(govRouter)
Expand Down
5 changes: 1 addition & 4 deletions x/gov/keeper/deposit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,8 @@ func (keeper Keeper) IterateDeposits(ctx context.Context, proposalID uint64, cb
// Activates voting period when appropriate and returns true in that case, else returns false.
func (keeper Keeper) AddDeposit(ctx context.Context, proposalID uint64, depositorAddr sdk.AccAddress, depositAmount sdk.Coins) (bool, error) {
// Checks to see if proposal exists
proposal, err := keeper.GetProposal(ctx, proposalID)
proposal, err := keeper.Proposals.Get(ctx, proposalID)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it isn't necessary to do a check of collections.ErrNotFound for keeping the same behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use the ErrNotFound of collections which is more descriptive and standardized. This breaks consensus but it does not break state compatibility so I think it should be fine, it also allowed us to remove some errors from gov which I think are a little overkill.

If you think this is not good I can revert the changes in a PR.

if errors.IsOf(err, types.ErrProposalNotFound) {
return false, errors.Wrapf(types.ErrUnknownProposal, "%d", proposalID)
}
return false, err
}

Expand Down
10 changes: 5 additions & 5 deletions x/gov/keeper/deposit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func TestDeposits(t *testing.T) {
// Check no deposits at beginning
_, err = govKeeper.Deposits.Get(ctx, collections.Join(proposalID, TestAddrs[1]))
require.ErrorIs(t, err, collections.ErrNotFound)
proposal, err = govKeeper.GetProposal(ctx, proposalID)
proposal, err = govKeeper.Proposals.Get(ctx, proposalID)
require.Nil(t, err)
require.Nil(t, proposal.VotingStartTime)

Expand All @@ -83,7 +83,7 @@ func TestDeposits(t *testing.T) {
require.Nil(t, err)
require.Equal(t, fourStake, sdk.NewCoins(deposit.Amount...))
require.Equal(t, TestAddrs[0].String(), deposit.Depositor)
proposal, err = govKeeper.GetProposal(ctx, proposalID)
proposal, err = govKeeper.Proposals.Get(ctx, proposalID)
require.Nil(t, err)
require.Equal(t, fourStake, sdk.NewCoins(proposal.TotalDeposit...))
require.Equal(t, addr0Initial.Sub(fourStake...), bankKeeper.GetAllBalances(ctx, TestAddrs[0]))
Expand All @@ -96,7 +96,7 @@ func TestDeposits(t *testing.T) {
require.Nil(t, err)
require.Equal(t, fourStake.Add(fiveStake...), sdk.NewCoins(deposit.Amount...))
require.Equal(t, TestAddrs[0].String(), deposit.Depositor)
proposal, err = govKeeper.GetProposal(ctx, proposalID)
proposal, err = govKeeper.Proposals.Get(ctx, proposalID)
require.Nil(t, err)
require.Equal(t, fourStake.Add(fiveStake...), sdk.NewCoins(proposal.TotalDeposit...))
require.Equal(t, addr0Initial.Sub(fourStake...).Sub(fiveStake...), bankKeeper.GetAllBalances(ctx, TestAddrs[0]))
Expand All @@ -109,13 +109,13 @@ func TestDeposits(t *testing.T) {
require.Nil(t, err)
require.Equal(t, TestAddrs[1].String(), deposit.Depositor)
require.Equal(t, fourStake, sdk.NewCoins(deposit.Amount...))
proposal, err = govKeeper.GetProposal(ctx, proposalID)
proposal, err = govKeeper.Proposals.Get(ctx, proposalID)
require.Nil(t, err)
require.Equal(t, fourStake.Add(fiveStake...).Add(fourStake...), sdk.NewCoins(proposal.TotalDeposit...))
require.Equal(t, addr1Initial.Sub(fourStake...), bankKeeper.GetAllBalances(ctx, TestAddrs[1]))

// Check that proposal moved to voting period
proposal, err = govKeeper.GetProposal(ctx, proposalID)
proposal, err = govKeeper.Proposals.Get(ctx, proposalID)
require.Nil(t, err)
require.True(t, proposal.VotingStartTime.Equal(ctx.BlockHeader().Time))

Expand Down
Loading