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.

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(x/protocol): remove Accounts.String() #19815

Merged
merged 5 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 6 additions & 1 deletion x/protocolpool/depinject.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,12 @@ func ProvideModule(in ModuleInputs) ModuleOutputs {
authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority)
}

k := keeper.NewKeeper(in.Codec, in.Environment, in.AccountKeeper, in.BankKeeper, in.StakingKeeper, authority.String())
authorityAddr, err := in.AccountKeeper.AddressCodec().BytesToString(authority)
if err != nil {
panic(err) // TODO
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
}
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved

k := keeper.NewKeeper(in.Codec, in.Environment, in.AccountKeeper, in.BankKeeper, in.StakingKeeper, authorityAddr)
m := NewAppModule(in.Codec, k, in.AccountKeeper, in.BankKeeper)

return ModuleOutputs{
Expand Down
12 changes: 10 additions & 2 deletions x/protocolpool/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,12 @@ func (k Keeper) InitGenesis(ctx context.Context, data *types.GenesisState) error
func (k Keeper) ExportGenesis(ctx context.Context) (*types.GenesisState, error) {
var cf []*types.ContinuousFund
err := k.ContinuousFund.Walk(ctx, nil, func(key sdk.AccAddress, value types.ContinuousFund) (stop bool, err error) {
recipient, err := k.authKeeper.AddressCodec().BytesToString(key)
if err != nil {
return true, err
}
cf = append(cf, &types.ContinuousFund{
Recipient: key.String(),
Recipient: recipient,
Percentage: value.Percentage,
Expiry: value.Expiry,
})
Expand All @@ -67,8 +71,12 @@ func (k Keeper) ExportGenesis(ctx context.Context) (*types.GenesisState, error)

var budget []*types.Budget
err = k.BudgetProposal.Walk(ctx, nil, func(key sdk.AccAddress, value types.Budget) (stop bool, err error) {
recipient, err := k.authKeeper.AddressCodec().BytesToString(key)
if err != nil {
return true, err
}
budget = append(budget, &types.Budget{
RecipientAddress: key.String(),
RecipientAddress: recipient,
TotalBudget: value.TotalBudget,
ClaimedAmount: value.ClaimedAmount,
StartTime: value.StartTime,
Expand Down
15 changes: 9 additions & 6 deletions x/protocolpool/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"cosmossdk.io/math"
"cosmossdk.io/x/protocolpool/types"

codectestutil "github.com/cosmos/cosmos-sdk/codec/testutil"
sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand All @@ -14,6 +15,8 @@ func (suite *KeeperTestSuite) TestUnclaimedBudget() {
period := time.Duration(60) * time.Second
zeroCoin := sdk.NewCoin("foo", math.ZeroInt())
nextClaimFrom := startTime.Add(period)
recipientStrAddr, err := codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(recipientAddr)
suite.Require().NoError(err)
testCases := []struct {
name string
preRun func()
Expand All @@ -34,7 +37,7 @@ func (suite *KeeperTestSuite) TestUnclaimedBudget() {
{
name: "no budget proposal found",
req: &types.QueryUnclaimedBudgetRequest{
Address: recipientAddr.String(),
Address: recipientStrAddr,
},
expErr: true,
expErrMsg: "no budget proposal found for address",
Expand All @@ -44,7 +47,7 @@ func (suite *KeeperTestSuite) TestUnclaimedBudget() {
preRun: func() {
// Prepare a valid budget proposal
budget := types.Budget{
RecipientAddress: recipientAddr.String(),
RecipientAddress: recipientStrAddr,
TotalBudget: &fooCoin,
StartTime: &startTime,
Tranches: 2,
Expand All @@ -54,7 +57,7 @@ func (suite *KeeperTestSuite) TestUnclaimedBudget() {
suite.Require().NoError(err)
},
req: &types.QueryUnclaimedBudgetRequest{
Address: recipientAddr.String(),
Address: recipientStrAddr,
},
expErr: false,
unclaimedFunds: &fooCoin,
Expand All @@ -72,7 +75,7 @@ func (suite *KeeperTestSuite) TestUnclaimedBudget() {
preRun: func() {
// Prepare a valid budget proposal
budget := types.Budget{
RecipientAddress: recipientAddr.String(),
RecipientAddress: recipientStrAddr,
TotalBudget: &fooCoin,
StartTime: &startTime,
Tranches: 2,
Expand All @@ -83,15 +86,15 @@ func (suite *KeeperTestSuite) TestUnclaimedBudget() {

// Claim the funds once
msg := &types.MsgClaimBudget{
RecipientAddress: recipientAddr.String(),
RecipientAddress: recipientStrAddr,
}
suite.mockSendCoinsFromModuleToAccount(recipientAddr)
_, err = suite.msgServer.ClaimBudget(suite.ctx, msg)
suite.Require().NoError(err)
},

req: &types.QueryUnclaimedBudgetRequest{
Address: recipientAddr.String(),
Address: recipientStrAddr,
},
expErr: false,
unclaimedFunds: &fooCoin2,
Expand Down
37 changes: 23 additions & 14 deletions x/protocolpool/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,16 +113,21 @@ func (k Keeper) GetCommunityPool(ctx context.Context) (sdk.Coins, error) {
return k.bankKeeper.GetAllBalances(ctx, moduleAccount.GetAddress()), nil
}

func (k Keeper) withdrawContinuousFund(ctx context.Context, recipient sdk.AccAddress) (sdk.Coin, error) {
func (k Keeper) withdrawContinuousFund(ctx context.Context, recipientAddr string) (sdk.Coin, error) {
recipient, err := k.authKeeper.AddressCodec().StringToBytes(recipientAddr)
if err != nil {
return sdk.Coin{}, sdkerrors.ErrInvalidAddress.Wrapf("invalid recipient address: %s", err)
}

cf, err := k.ContinuousFund.Get(ctx, recipient)
if err != nil {
if errors.Is(err, collections.ErrNotFound) {
return sdk.Coin{}, fmt.Errorf("no continuous fund found for recipient: %s", recipient.String())
return sdk.Coin{}, fmt.Errorf("no continuous fund found for recipient: %s", recipientAddr)
}
return sdk.Coin{}, fmt.Errorf("get continuous fund failed for recipient: %s", recipient.String())
return sdk.Coin{}, fmt.Errorf("get continuous fund failed for recipient: %s", recipientAddr)
}
if cf.Expiry != nil && cf.Expiry.Before(k.environment.HeaderService.GetHeaderInfo(ctx).Time) {
return sdk.Coin{}, fmt.Errorf("cannot withdraw continuous funds: continuous fund expired for recipient: %s", recipient.String())
return sdk.Coin{}, fmt.Errorf("cannot withdraw continuous funds: continuous fund expired for recipient: %s", recipientAddr)
}

toDistributeAmount, err := k.ToDistribute.Get(ctx)
Expand All @@ -138,15 +143,15 @@ func (k Keeper) withdrawContinuousFund(ctx context.Context, recipient sdk.AccAdd
}

// withdraw continuous fund
withdrawnAmount, err := k.withdrawRecipientFunds(ctx, recipient)
withdrawnAmount, err := k.withdrawRecipientFunds(ctx, recipient, recipientAddr)
if err != nil {
return sdk.Coin{}, fmt.Errorf("error while withdrawing recipient funds for recipient: %s", recipient.String())
return sdk.Coin{}, fmt.Errorf("error while withdrawing recipient funds for recipient: %s", recipientAddr)
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
}

return withdrawnAmount, nil
}

func (k Keeper) withdrawRecipientFunds(ctx context.Context, recipient sdk.AccAddress) (sdk.Coin, error) {
func (k Keeper) withdrawRecipientFunds(ctx context.Context, recipient sdk.AccAddress, recipientAddr string) (sdk.Coin, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, maintain consistency

// get allocated continuous fund
fundsAllocated, err := k.RecipientFundDistribution.Get(ctx, recipient)
if err != nil {
Expand All @@ -165,7 +170,7 @@ func (k Keeper) withdrawRecipientFunds(ctx context.Context, recipient sdk.AccAdd
withdrawnAmount := sdk.NewCoin(denom, fundsAllocated)
err = k.DistributeFromStreamFunds(ctx, sdk.NewCoins(withdrawnAmount), recipient)
if err != nil {
return sdk.Coin{}, fmt.Errorf("error while distributing funds to the recipient %s: %v", recipient.String(), err)
return sdk.Coin{}, fmt.Errorf("error while distributing funds to the recipient %s: %v", recipientAddr, err)
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
}

// reset fund distribution
Expand Down Expand Up @@ -257,8 +262,12 @@ func (k Keeper) iterateAndUpdateFundsDistribution(ctx context.Context, toDistrib

// Calculate totalPercentageToBeDistributed and store values
err := k.RecipientFundPercentage.Walk(ctx, nil, func(key sdk.AccAddress, value math.Int) (stop bool, err error) {
addr, err := k.authKeeper.AddressCodec().BytesToString(key)
if err != nil {
return true, err
}
totalPercentageToBeDistributed = totalPercentageToBeDistributed.Add(value)
recipientFundMap[key.String()] = value
recipientFundMap[addr] = value
return false, nil
})
if err != nil {
Expand Down Expand Up @@ -306,9 +315,9 @@ func (k Keeper) iterateAndUpdateFundsDistribution(ctx context.Context, toDistrib
return k.ToDistribute.Set(ctx, math.ZeroInt())
}

func (k Keeper) claimFunds(ctx context.Context, recipient sdk.AccAddress) (amount sdk.Coin, err error) {
func (k Keeper) claimFunds(ctx context.Context, recipient sdk.AccAddress, recipientAddr string) (amount sdk.Coin, err error) {
JulianToledano marked this conversation as resolved.
Show resolved Hide resolved
// get claimable funds from distribution info
amount, err = k.getClaimableFunds(ctx, recipient)
amount, err = k.getClaimableFunds(ctx, recipient, recipientAddr)
if err != nil {
return sdk.Coin{}, fmt.Errorf("error getting claimable funds: %w", err)
}
Expand All @@ -322,11 +331,11 @@ func (k Keeper) claimFunds(ctx context.Context, recipient sdk.AccAddress) (amoun
return amount, nil
}

func (k Keeper) getClaimableFunds(ctx context.Context, recipient sdk.AccAddress) (amount sdk.Coin, err error) {
func (k Keeper) getClaimableFunds(ctx context.Context, recipient sdk.AccAddress, recipientAddr string) (amount sdk.Coin, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is called from claimFunds, which just converted the string address to sdk.AccAddress. If both are not passed, we just have to convert it again.
I'm not against it, but I don't know how performance can be affected.

Copy link
Contributor

Choose a reason for hiding this comment

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

As it doesn't involve complex operations and inconsistencies, I think we could just call it twice to maintain consistency with other functions

budget, err := k.BudgetProposal.Get(ctx, recipient)
if err != nil {
if errors.Is(err, collections.ErrNotFound) {
return sdk.Coin{}, fmt.Errorf("no budget found for recipient: %s", recipient.String())
return sdk.Coin{}, fmt.Errorf("no budget found for recipient: %s", recipientAddr)
}
return sdk.Coin{}, err
}
Expand All @@ -340,7 +349,7 @@ func (k Keeper) getClaimableFunds(ctx context.Context, recipient sdk.AccAddress)
return sdk.Coin{}, err
}
// Return the end of the budget
return sdk.Coin{}, fmt.Errorf("budget ended for recipient: %s", recipient.String())
return sdk.Coin{}, fmt.Errorf("budget ended for recipient: %s", recipientAddr)
}
}

Expand Down
5 changes: 4 additions & 1 deletion x/protocolpool/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,16 @@ func (s *KeeperTestSuite) SetupTest() {
stakingKeeper.EXPECT().BondDenom(ctx).Return("stake", nil).AnyTimes()
s.stakingKeeper = stakingKeeper

authority, err := accountKeeper.AddressCodec().BytesToString(authtypes.NewModuleAddress(types.GovModuleName))
s.Require().NoError(err)

poolKeeper := poolkeeper.NewKeeper(
encCfg.Codec,
environment,
accountKeeper,
bankKeeper,
stakingKeeper,
authtypes.NewModuleAddress(types.GovModuleName).String(),
authority,
)
s.ctx = ctx
s.poolKeeper = poolKeeper
Expand Down
11 changes: 3 additions & 8 deletions x/protocolpool/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func (k MsgServer) ClaimBudget(ctx context.Context, msg *types.MsgClaimBudget) (
return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid recipient address: %s", err)
}

amount, err := k.claimFunds(ctx, recipient)
amount, err := k.claimFunds(ctx, recipient, msg.RecipientAddress)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -164,12 +164,7 @@ func (k MsgServer) CreateContinuousFund(ctx context.Context, msg *types.MsgCreat
}

func (k MsgServer) WithdrawContinuousFund(ctx context.Context, msg *types.MsgWithdrawContinuousFund) (*types.MsgWithdrawContinuousFundResponse, error) {
recipient, err := k.Keeper.authKeeper.AddressCodec().StringToBytes(msg.RecipientAddress)
if err != nil {
return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid recipient address: %s", err)
}

amount, err := k.withdrawContinuousFund(ctx, recipient)
amount, err := k.withdrawContinuousFund(ctx, msg.RecipientAddress)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -202,7 +197,7 @@ func (k MsgServer) CancelContinuousFund(ctx context.Context, msg *types.MsgCance
}

// withdraw funds if any are allocated
withdrawnFunds, err := k.withdrawRecipientFunds(ctx, recipient)
withdrawnFunds, err := k.withdrawRecipientFunds(ctx, recipient, msg.RecipientAddress)
if err != nil && !errorspkg.Is(err, types.ErrNoRecipientFund) {
return nil, fmt.Errorf("error while withdrawing already allocated funds for recipient %s: %v", msg.RecipientAddress, err)
}
Expand Down
Loading
Loading