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

[Testing] add account balance coverage to account settlement #670

Merged
merged 3 commits into from
Jul 15, 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
2 changes: 2 additions & 0 deletions testutil/keeper/tokenomics.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type TokenomicsModuleKeepers struct {
tokenomicstypes.AccountKeeper
tokenomicstypes.BankKeeper
tokenomicstypes.ApplicationKeeper
tokenomicstypes.SupplierKeeper
tokenomicstypes.ProofKeeper
tokenomicstypes.SharedKeeper

Expand Down Expand Up @@ -340,6 +341,7 @@ func NewTokenomicsModuleKeepers(
AccountKeeper: &accountKeeper,
BankKeeper: &bankKeeper,
ApplicationKeeper: &appKeeper,
SupplierKeeper: &supplierKeeper,
ProofKeeper: &proofKeeper,
SharedKeeper: &sharedKeeper,

Expand Down
216 changes: 203 additions & 13 deletions x/tokenomics/keeper/settle_session_accounting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ package keeper_test

import (
"bytes"
"context"
"fmt"
"testing"

"cosmossdk.io/math"
"github.com/cosmos/cosmos-sdk/types"
cosmostypes "github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
"github.com/pokt-network/smt"
"github.com/stretchr/testify/require"

Expand All @@ -18,26 +21,30 @@ import (
prooftypes "github.com/pokt-network/poktroll/x/proof/types"
sessiontypes "github.com/pokt-network/poktroll/x/session/types"
sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
suppliertypes "github.com/pokt-network/poktroll/x/supplier/types"
tokenomicstypes "github.com/pokt-network/poktroll/x/tokenomics/types"
)

func TestSettleSessionAccounting_HandleAppGoingIntoDebt(t *testing.T) {
t.Skip("TODO_MAINNET: Add coverage of the design choice made for how to handle this scenario.")
Copy link

Choose a reason for hiding this comment

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

Reminder: Add coverage for the design choice.

The test is currently skipped with a TODO comment. Ensure to add the necessary test coverage for the design choice made for handling this scenario.

Do you want me to help with adding the test coverage or open a GitHub issue to track this task?


keepers, ctx := testkeeper.NewTokenomicsModuleKeepers(t, nil)

// Add a new application
appStake := types.NewCoin("upokt", math.NewInt(1000000))
appStake := cosmostypes.NewCoin("upokt", math.NewInt(1000000))
app := apptypes.Application{
Address: sample.AccAddress(),
Stake: &appStake,
}
keepers.SetApplication(ctx, app)

// Add a new supplier
supplierStake := types.NewCoin("upokt", math.NewInt(1000000))
supplierStake := cosmostypes.NewCoin("upokt", math.NewInt(1000000))
supplier := sharedtypes.Supplier{
Address: sample.AccAddress(),
Stake: &supplierStake,
}
keepers.SetSupplier(ctx, supplier)

// The base claim whose root will be customized for testing purposes
claim := prooftypes.Claim{
Expand All @@ -57,24 +64,188 @@ func TestSettleSessionAccounting_HandleAppGoingIntoDebt(t *testing.T) {

err := keepers.SettleSessionAccounting(ctx, &claim)
require.NoError(t, err)
// TODO_TEST: Need to make sure the application is unstaked at this point in time.
}

func TestSettleSessionAccounting_ValidAccounting(t *testing.T) {
t.Skip("TODO_BLOCKER(@Olshansk): Add E2E and integration tests so we validate the actual state changes of the bank & account keepers.")
// Assert that `suppliertypes.ModuleName` account module balance is *unchanged*
// Assert that `supplierAddress` account balance has *increased* by the appropriate amount
// Assert that `supplierAddress` staked balance is *unchanged*
// Assert that `apptypes.ModuleName` account module balance is *unchanged*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Reviewer, this assertion seems to be incorrect to me. My understanding is that the account module balance MUST decrease as a result of the burn. I assumed this is copy-pasta and have the test making the modified assertion.

keepers, ctx := testkeeper.NewTokenomicsModuleKeepers(t, nil)
appModuleAddress := authtypes.NewModuleAddress(apptypes.ModuleName).String()
supplierModuleAddress := authtypes.NewModuleAddress(suppliertypes.ModuleName).String()

// Set compute_units_to_tokens_multiplier to 1 to simplify expectation calculations.
err := keepers.Keeper.SetParams(ctx, tokenomicstypes.Params{
ComputeUnitsToTokensMultiplier: 1,
})
require.NoError(t, err)

// Add a new application
appStake := cosmostypes.NewCoin("upokt", math.NewInt(1000000))
expectedAppEndStakeAmount := cosmostypes.NewCoin("upokt", math.NewInt(420))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we calculate this value or explain how we came up with it?

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 just an arbitrary amount which the expectedAppBurn is defined in terms of. The idea is to ensure a non-zero stake balance that can be asserted at the end of the test. Defining this value first lets us define expectedAppBurn in terms of it and appStake, which I figured would be the simplest way to set things up.

expectedAppBurn := appStake.Sub(expectedAppEndStakeAmount)
app := apptypes.Application{
Address: sample.AccAddress(),
Stake: &appStake,
}
keepers.SetApplication(ctx, app)

// Query application balance prior to the accounting.
appStartBalance := getBalance(t, ctx, keepers, app.GetAddress())

// Query application module balance prior to the accounting.
appModuleStartBalance := getBalance(t, ctx, keepers, appModuleAddress)

// Add a new supplier.
supplierStake := cosmostypes.NewCoin("upokt", math.NewInt(1000000))
supplier := sharedtypes.Supplier{
Address: sample.AccAddress(),
Stake: &supplierStake,
}
keepers.SetSupplier(ctx, supplier)

// Query supplier balance prior to the accounting.
supplierStartBalance := getBalance(t, ctx, keepers, supplier.GetAddress())

// Query supplier module balance prior to the accounting.
supplierModuleStartBalance := getBalance(t, ctx, keepers, supplierModuleAddress)

// The base claim whose root will be customized for testing purposes
claim := prooftypes.Claim{
SupplierAddress: supplier.Address,
SessionHeader: &sessiontypes.SessionHeader{
ApplicationAddress: app.Address,
Service: &sharedtypes.Service{
Id: "svc1",
Name: "svcName1",
},
SessionId: "session_id",
SessionStartBlockHeight: 1,
SessionEndBlockHeight: testsession.GetSessionEndHeightWithDefaultParams(1),
},
RootHash: testproof.SmstRootWithSum(expectedAppBurn.Amount.Uint64()),
}

err = keepers.SettleSessionAccounting(ctx, &claim)
require.NoError(t, err)

// Assert that `applicationAddress` account balance is *unchanged*
appEndBalance := getBalance(t, ctx, keepers, app.GetAddress())
require.EqualValues(t, appStartBalance, appEndBalance)

// Assert that `applicationAddress` staked balance has decreased by the appropriate amount
app, appIsFound := keepers.GetApplication(ctx, app.GetAddress())
require.True(t, appIsFound)
require.Equal(t, &expectedAppEndStakeAmount, app.GetStake())

// Assert that `apptypes.ModuleName` account module balance is *decreased* by the appropriate amount
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment on why app module balance decreases and not supplier module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

	// Assert that `apptypes.ModuleName` account module balance is *decreased* by the appropriate amount
	// NB: The application module account burns the amount of uPOKT that was held in escrow
	// on behalf of the applications which were serviced in a given session.
	// Assert that `suppliertypes.ModuleName` account module balance is *unchanged*
	// NB: Supplier rewards are minted to the supplier module account but then immediately
	// distributed to the supplier accounts which provided service in a given session.

appModuleEndBalance := getBalance(t, ctx, keepers, appModuleAddress)
expectedAppModuleEndBalance := appModuleStartBalance.Sub(expectedAppBurn)
require.NotNil(t, appModuleEndBalance)
require.EqualValues(t, &expectedAppModuleEndBalance, appModuleEndBalance)

// Assert that `supplierAddress` account balance has *increased* by the appropriate amount
supplierEndBalance := getBalance(t, ctx, keepers, supplier.GetAddress())
expectedSupplierBalance := supplierStartBalance.Add(expectedAppBurn)
require.EqualValues(t, &expectedSupplierBalance, supplierEndBalance)

// Assert that `supplierAddress` staked balance is *unchanged*
supplier, supplierIsFound := keepers.GetSupplier(ctx, supplier.GetAddress())
require.True(t, supplierIsFound)
require.Equal(t, &supplierStake, supplier.GetStake())

// Assert that `suppliertypes.ModuleName` account module balance is *unchanged*
supplierModuleEndBalance := getBalance(t, ctx, keepers, supplierModuleAddress)
require.EqualValues(t, supplierModuleStartBalance, supplierModuleEndBalance)
}

func TestSettleSessionAccounting_AppStakeTooLow(t *testing.T) {
t.Skip("TODO_BLOCKER(@Olshansk): Add E2E and integration tests so we validate the actual state changes of the bank & account keepers.")
// Assert that `suppliertypes.Address` account balance has *increased* by the appropriate amount
// Assert that `applicationAddress` account staked balance has gone to zero
// Assert on whatever logic we have for slashing the application or other
keepers, ctx := testkeeper.NewTokenomicsModuleKeepers(t, nil)
appModuleAddress := authtypes.NewModuleAddress(apptypes.ModuleName).String()
supplierModuleAddress := authtypes.NewModuleAddress(suppliertypes.ModuleName).String()

// Set compute_units_to_tokens_multiplier to 1 to simplify expectation calculations.
err := keepers.Keeper.SetParams(ctx, tokenomicstypes.Params{
ComputeUnitsToTokensMultiplier: 1,
})
require.NoError(t, err)

// Add a new application
appStake := cosmostypes.NewCoin("upokt", math.NewInt(40000))
expectedAppEndStakeAmount := cosmostypes.NewCoin("upokt", math.NewInt(0))
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
expectedAppBurn := appStake.AddAmount(math.NewInt(2000))
app := apptypes.Application{
Address: sample.AccAddress(),
Stake: &appStake,
}
keepers.SetApplication(ctx, app)

// Query application balance prior to the accounting.
appStartBalance := getBalance(t, ctx, keepers, app.GetAddress())

// Query application module balance prior to the accounting.
appModuleStartBalance := getBalance(t, ctx, keepers, appModuleAddress)

// Add a new supplier.
supplierStake := cosmostypes.NewCoin("upokt", math.NewInt(1000000))
supplier := sharedtypes.Supplier{
Address: sample.AccAddress(),
Stake: &supplierStake,
}
keepers.SetSupplier(ctx, supplier)

// Query supplier balance prior to the accounting.
supplierStartBalance := getBalance(t, ctx, keepers, supplier.GetAddress())

// Query supplier module balance prior to the accounting.
supplierModuleStartBalance := getBalance(t, ctx, keepers, supplierModuleAddress)

// The base claim whose root will be customized for testing purposes
claim := prooftypes.Claim{
SupplierAddress: supplier.Address,
SessionHeader: &sessiontypes.SessionHeader{
ApplicationAddress: app.Address,
Service: &sharedtypes.Service{
Id: "svc1",
Name: "svcName1",
},
SessionId: "session_id",
SessionStartBlockHeight: 1,
SessionEndBlockHeight: testsession.GetSessionEndHeightWithDefaultParams(1),
},
RootHash: testproof.SmstRootWithSum(expectedAppBurn.Amount.Uint64()),
}

err = keepers.SettleSessionAccounting(ctx, &claim)
require.NoError(t, err)

// Assert that `applicationAddress` account balance is *unchanged*
appEndBalance := getBalance(t, ctx, keepers, app.GetAddress())
require.EqualValues(t, appStartBalance, appEndBalance)

// Assert that `applicationAddress` staked balance has gone to zero
app, appIsFound := keepers.GetApplication(ctx, app.GetAddress())
require.True(t, appIsFound)
require.Equal(t, &expectedAppEndStakeAmount, app.GetStake())

// Assert that `apptypes.ModuleName` account module balance is *decreased* by the appropriate amount
appModuleEndBalance := getBalance(t, ctx, keepers, appModuleAddress)
expectedAppModuleEndBalance := appModuleStartBalance.Sub(appStake)
require.NotNil(t, appModuleEndBalance)
require.EqualValues(t, &expectedAppModuleEndBalance, appModuleEndBalance)

// Assert that `supplierAddress` account balance has *increased* by the appropriate amount
supplierEndBalance := getBalance(t, ctx, keepers, supplier.GetAddress())
require.NotNil(t, supplierEndBalance)

expectedSupplierBalance := supplierStartBalance.Add(expectedAppBurn)
require.EqualValues(t, &expectedSupplierBalance, supplierEndBalance)

// Assert that `supplierAddress` staked balance is *unchanged*
supplier, supplierIsFound := keepers.GetSupplier(ctx, supplier.GetAddress())
require.True(t, supplierIsFound)
require.Equal(t, &supplierStake, supplier.GetStake())

// Assert that `suppliertypes.ModuleName` account module balance is *unchanged*
supplierModuleEndBalance := getBalance(t, ctx, keepers, supplierModuleAddress)
require.EqualValues(t, supplierModuleStartBalance, supplierModuleEndBalance)
}

func TestSettleSessionAccounting_AppNotFound(t *testing.T) {
Expand Down Expand Up @@ -282,3 +453,22 @@ func TestSettleSessionAccounting_InvalidClaim(t *testing.T) {
})
}
}

func getBalance(
t *testing.T,
ctx context.Context,
bankKeeper tokenomicstypes.BankKeeper,
accountAddr string,
) *cosmostypes.Coin {

appBalanceRes, err := bankKeeper.Balance(ctx, &banktypes.QueryBalanceRequest{
Address: accountAddr,
Denom: "upokt",
})
require.NoError(t, err)

balance := appBalanceRes.GetBalance()
require.NotNil(t, balance)

return balance
}
7 changes: 7 additions & 0 deletions x/tokenomics/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"context"

sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

apptypes "github.com/pokt-network/poktroll/x/application/types"
prooftypes "github.com/pokt-network/poktroll/x/proof/types"
Expand All @@ -26,6 +27,7 @@ type BankKeeper interface {
// than "delegate" funds from one account to another which is more closely
// linked to staking.
SendCoinsFromModuleToAccount(ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error
Balance(context.Context, *banktypes.QueryBalanceRequest) (*banktypes.QueryBalanceResponse, error)
}

type ApplicationKeeper interface {
Expand Down Expand Up @@ -53,3 +55,8 @@ type SharedKeeper interface {

GetProofWindowCloseHeight(ctx context.Context, queryHeight int64) int64
}

type SupplierKeeper interface {
GetSupplier(ctx context.Context, supplierAddr string) (supplier sharedtypes.Supplier, found bool)
SetSupplier(ctx context.Context, supplier sharedtypes.Supplier)
}
Loading