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

[Gateway] Enforce minimum stake when staking #843

Merged
merged 53 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
d1cd5e6
scaffold: message update-param --module gateway --signer authority na…
bryanchriswhite Sep 9, 2024
91eecfc
chore: update gateway MsgUpdateParam fields
bryanchriswhite Sep 9, 2024
ef62280
chore: update MsgUpdateParamResponse fields
bryanchriswhite Sep 23, 2024
f7a6e5b
chore: comment out autocli for gateway module param updates
bryanchriswhite Sep 10, 2024
8957f1c
fix: linter error
bryanchriswhite Sep 26, 2024
3016ca3
revert: premature changes
bryanchriswhite Sep 26, 2024
899effb
feat: add min_stake gateway module param
bryanchriswhite Sep 26, 2024
83c1d74
chore: cleanup makefiles and param update JSON files
bryanchriswhite Sep 23, 2024
55e6a31
chore: add gateway module param make targets
bryanchriswhite Sep 23, 2024
ddd68bb
refctor: consolidate param config types
bryanchriswhite Sep 26, 2024
fa8caf6
tests: improve error messaging
bryanchriswhite Sep 26, 2024
ae006e2
tests: add coverage over gateway min stake param updates
bryanchriswhite Sep 26, 2024
e92e3b1
chore: add as_coin as_type type 😉
bryanchriswhite Sep 26, 2024
b2e1267
chore: add app MsgUpdateParam to genesis authorizations
bryanchriswhite Sep 26, 2024
0373fa2
fix: typo
bryanchriswhite Sep 26, 2024
da7894a
chore: cleanup makefiles
bryanchriswhite Sep 26, 2024
015c537
wip: fixing tests
bryanchriswhite Sep 26, 2024
1ac6c48
tests: add tests
bryanchriswhite Sep 26, 2024
db366f6
tests: remove erroneous case
bryanchriswhite Sep 26, 2024
60bef17
test: gateway staking below minimum fails
bryanchriswhite Sep 27, 2024
a21687f
feat: gateway min stake validation & grpc status error returns
bryanchriswhite Sep 27, 2024
7e9f857
chore: cleanup comments
bryanchriswhite Sep 27, 2024
4340590
chore: review feedback improvements
bryanchriswhite Sep 30, 2024
4b8e95b
Merge branch 'main' into issues/612/chore/gateway-msg-update-param
bryanchriswhite Sep 30, 2024
0839ae4
chore: review feedback improvements
bryanchriswhite Sep 30, 2024
34b0d6d
Merge remote-tracking branch 'pokt/issues/612/chore/gateway-msg-updat…
bryanchriswhite Sep 30, 2024
f48fa66
Merge branch 'issues/612/param/min-stake-gateway' into issues/612/gat…
bryanchriswhite Sep 30, 2024
6c5e13f
chore: review feedback improvements
bryanchriswhite Sep 30, 2024
a77877b
fix: test error message
bryanchriswhite Sep 30, 2024
9f65d25
Merge branch 'issues/612/param/min-stake-gateway' into issues/612/gat…
bryanchriswhite Sep 30, 2024
28962ba
Merge remote-tracking branch 'pokt/main' into issues/612/param/min-st…
bryanchriswhite Sep 30, 2024
891a858
chore: review feedback improvements
bryanchriswhite Oct 2, 2024
08e99ba
Merge branch 'issues/612/param/min-stake-gateway' into issues/612/gat…
bryanchriswhite Oct 2, 2024
ccc3f53
fix: e2e test
bryanchriswhite Oct 2, 2024
47e0993
Merge branch 'issues/612/param/min-stake-gateway' into issues/612/gat…
bryanchriswhite Oct 2, 2024
b29843e
Empty commit
bryanchriswhite Oct 2, 2024
8c25061
Empty commit
bryanchriswhite Oct 2, 2024
3f59041
Merge branch 'issues/612/param/min-stake-gateway' into issues/612/gat…
bryanchriswhite Oct 2, 2024
4e259c9
chore: apply improvements
bryanchriswhite Oct 2, 2024
2808925
chore: add godoc style comment to min_stake params field
bryanchriswhite Oct 2, 2024
3280142
chore: improve logging & ensure gRPC status error returns
bryanchriswhite Oct 4, 2024
c256eae
chore: review feedback improvements
bryanchriswhite Oct 4, 2024
0d36e0f
Merge remote-tracking branch 'pokt/main' into issues/612/param/min-st…
bryanchriswhite Oct 4, 2024
00914b0
Merge branch 'issues/612/param/min-stake-gateway' into issues/612/gat…
bryanchriswhite Oct 4, 2024
1c309b8
Empty commit
bryanchriswhite Oct 4, 2024
e7ff959
Merge branch 'issues/612/param/min-stake-gateway' into issues/612/gat…
bryanchriswhite Oct 4, 2024
1257f6e
Merge branch 'main' into issues/612/gateway/logic
bryanchriswhite Oct 4, 2024
4d2c2d9
chore: review feedback improvements
bryanchriswhite Oct 4, 2024
5ba2a48
fix: typo
bryanchriswhite Oct 4, 2024
3e4ef9e
fix: linter errors
bryanchriswhite Oct 4, 2024
cd5e420
chore: review feedback improvements
bryanchriswhite Oct 4, 2024
65158ad
Merge branch 'main' into issues/612/gateway/logic
bryanchriswhite Oct 4, 2024
698991a
Empty commit
bryanchriswhite Oct 4, 2024
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
57 changes: 40 additions & 17 deletions x/gateway/keeper/msg_server_stake_gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/pokt-network/poktroll/telemetry"
"github.com/pokt-network/poktroll/x/gateway/types"
Expand All @@ -24,46 +26,67 @@ func (k msgServer) StakeGateway(
ctx := sdk.UnwrapSDKContext(goCtx)

logger := k.Logger().With("method", "StakeGateway")
logger.Info(fmt.Sprintf("About to stake gateway with msg: %v", msg))
logger.Info(fmt.Sprintf("about to stake gateway with msg: %v", msg))

if err := msg.ValidateBasic(); err != nil {
return nil, err
return nil, status.Error(codes.InvalidArgument, err.Error())
}

// Retrieve the address of the gateway
gatewayAddress, err := sdk.AccAddressFromBech32(msg.Address)
// NB: This SHOULD NEVER happen because msg.ValidateBasic() validates the address as bech32.
if err != nil {
// TODO_TECHDEBT(#384): determine whether to continue using cosmos logger for debug level.
logger.Info(fmt.Sprintf("could not parse address %q", msg.Address))
Copy link
Member

Choose a reason for hiding this comment

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

This should either be an error or a warning. Please also log the error in there as well

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 have a strong and principled opinion here. I'm confident that we've discussed this before but it's been a while so please let me reiterate my thinking on this:

  1. In my opinion, error, warn, and even info logs should be reserved for logs which are relevant to the service operator; as opposed to the end user, for example. These logs should be important, actionable, or indicative of a status change of the service.

    The rationale here is that in the absence of such prescriptive usage of the log levels, logs which are relevant to the service operator are lost in the noise of irrelevant logs.

  2. Debug logs should be used for everything else. In all the cases where I've been switching log levels, these logs are not intended for the validator operator but instead for protocol developers during development and testing.

  3. Since the cosmos-sdk debug level is practically useless (full of cosmos-sdk internal logs), we chose to use the info level for debug logs on-chain.

  4. The long-term solution I was imagining was to add a cosmos-sdk logger backed implementation of the polylog interface that would shunt the existing debug level logs to a new level (e.g. cosmos-debug or something) and give us back a useful debug level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the [Logging] section of #384 is outstanding.

Regarding automating/improving alignment with this approach: I could imagine a CI task which collects all the non debug logs added (plus a few lines of context) in a PR and leaves them in a comment.

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Oct 4, 2024

Choose a reason for hiding this comment

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

I've updated the logs that I changed to follow this pattern:

err := examplemodtypes.ErrExamplemodSomeError.Wrapf("fmt string ...", ...fmtArgs)
logger.Info(fmt.Sprintf("ERROR: %s", err)
return nil, status.Error(codes.InvalidArgument, error.Error())

bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
return nil, status.Error(codes.InvalidArgument, err.Error())
}

// Check if the gateway already exists or not
var err error
var coinsToEscrow sdk.Coin
gateway, isGatewayFound := k.GetGateway(ctx, msg.Address)
if !isGatewayFound {
logger.Info(fmt.Sprintf("Gateway not found. Creating new gateway for address %q", msg.Address))
logger.Info(fmt.Sprintf("gateway not found. Creating new gateway for address %q", msg.Address))
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
gateway = k.createGateway(ctx, msg)
coinsToEscrow = *msg.Stake
} else {
logger.Info(fmt.Sprintf("Gateway found. About to try and update gateway for address %q", msg.Address))
logger.Info(fmt.Sprintf("gateway found. About to try and update gateway for address %q", msg.Address))
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
currGatewayStake := *gateway.Stake
if err = k.updateGateway(ctx, &gateway, msg); err != nil {
logger.Error(fmt.Sprintf("could not update gateway for address %q due to error %v", msg.Address, err))
return nil, err
return nil, status.Error(codes.InvalidArgument, err.Error())
}
coinsToEscrow, err = (*msg.Stake).SafeSub(currGatewayStake)
if err != nil {
return nil, err
return nil, status.Error(
codes.InvalidArgument,
types.ErrGatewayInvalidStake.Wrapf(
"stake (%s) must be higher than previous stake (%s)",
msg.Stake, currGatewayStake,
).Error(),
)
}
logger.Info(fmt.Sprintf("Gateway is going to escrow an additional %+v coins", coinsToEscrow))
logger.Info(fmt.Sprintf("gateway is going to escrow an additional %+v coins", coinsToEscrow))
}

// Must always stake or upstake (> 0 delta)
// MUST ALWAYS stake or upstake (> 0 delta).
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
if coinsToEscrow.IsZero() {
logger.Warn(fmt.Sprintf("Gateway %q must escrow more than 0 additional coins", msg.Address))
return nil, types.ErrGatewayInvalidStake.Wrapf("gateway %q must escrow more than 0 additional coins", msg.Address)
errMsg := fmt.Sprintf("gateway %q must escrow more than 0 additional coins", msg.GetAddress())
logger.Info(errMsg)
return nil, status.Error(
codes.InvalidArgument,
types.ErrGatewayInvalidStake.Wrap(errMsg).Error(),
)
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
}

// Retrieve the address of the gateway
gatewayAddress, err := sdk.AccAddressFromBech32(msg.Address)
if err != nil {
// TODO_TECHDEBT(#384): determine whether to continue using cosmos logger for debug level.
logger.Error(fmt.Sprintf("could not parse address %q", msg.Address))
return nil, err
// MUST ALWAYS have at least minimum stake.
minStake := k.GetParams(ctx).MinStake
if msg.Stake.Amount.LT(minStake.Amount) {
errFmt := "gateway %q must stake at least %s"
logger.Info(fmt.Sprintf(errFmt, msg.Address, minStake))
Copy link
Member

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.

return nil, status.Error(
codes.InvalidArgument,
types.ErrGatewayInvalidStake.Wrapf(errFmt, msg.Address, minStake).Error(),
)
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
}

// Send the coins from the gateway to the staked gateway pool
Expand Down
73 changes: 51 additions & 22 deletions x/gateway/keeper/msg_server_stake_gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,51 +4,52 @@ import (
"testing"

"cosmossdk.io/math"
sdk "github.com/cosmos/cosmos-sdk/types"
cosmostypes "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/require"

"github.com/pokt-network/poktroll/app/volatile"
keepertest "github.com/pokt-network/poktroll/testutil/keeper"
"github.com/pokt-network/poktroll/testutil/sample"
"github.com/pokt-network/poktroll/x/gateway/keeper"
"github.com/pokt-network/poktroll/x/gateway/types"
gatewaytypes "github.com/pokt-network/poktroll/x/gateway/types"
)

func TestMsgServer_StakeGateway_SuccessfulCreateAndUpdate(t *testing.T) {
k, ctx := keepertest.GatewayKeeper(t)
srv := keeper.NewMsgServerImpl(k)

// Generate an address for the gateway
// Generate an address for the gateway.
addr := sample.AccAddress()

// Verify that the gateway does not exist yet
// Verify that the gateway does not exist yet.
_, isGatewayFound := k.GetGateway(ctx, addr)
require.False(t, isGatewayFound)

// Prepare the gateway
initialStake := sdk.NewCoin("upokt", math.NewInt(100))
stakeMsg := &types.MsgStakeGateway{
// Prepare the gateway.
initialStake := cosmostypes.NewCoin("upokt", math.NewInt(100))
stakeMsg := &gatewaytypes.MsgStakeGateway{
Address: addr,
Stake: &initialStake,
}

// Stake the gateway
// Stake the gateway.
_, err := srv.StakeGateway(ctx, stakeMsg)
require.NoError(t, err)

// Verify that the gateway exists
// Verify that the gateway exists.
foundGateway, isGatewayFound := k.GetGateway(ctx, addr)
require.True(t, isGatewayFound)
require.Equal(t, addr, foundGateway.Address)
require.Equal(t, initialStake.Amount, foundGateway.Stake.Amount)

// Prepare an updated gateway with a higher stake
updatedStake := sdk.NewCoin("upokt", math.NewInt(200))
updateMsg := &types.MsgStakeGateway{
// Prepare an updated gateway with a higher stake.
updatedStake := cosmostypes.NewCoin("upokt", math.NewInt(200))
updateMsg := &gatewaytypes.MsgStakeGateway{
Address: addr,
Stake: &updatedStake,
}

// Update the staked gateway
// Update the staked gateway.
_, err = srv.StakeGateway(ctx, updateMsg)
require.NoError(t, err)
foundGateway, isGatewayFound = k.GetGateway(ctx, addr)
Expand All @@ -60,33 +61,61 @@ func TestMsgServer_StakeGateway_FailLoweringStake(t *testing.T) {
k, ctx := keepertest.GatewayKeeper(t)
srv := keeper.NewMsgServerImpl(k)

// Prepare the gateway
// Prepare the gateway.
addr := sample.AccAddress()
initialStake := sdk.NewCoin("upokt", math.NewInt(100))
stakeMsg := &types.MsgStakeGateway{
initialStake := cosmostypes.NewCoin("upokt", math.NewInt(100))
stakeMsg := &gatewaytypes.MsgStakeGateway{
Address: addr,
Stake: &initialStake,
}

// Stake the gateway & verify that the gateway exists
// Stake the gateway & verify that the gateway exists.
_, err := srv.StakeGateway(ctx, stakeMsg)
require.NoError(t, err)
_, isGatewayFound := k.GetGateway(ctx, addr)
require.True(t, isGatewayFound)

// Prepare an updated gateway with a lower stake
updatedStake := sdk.NewCoin("upokt", math.NewInt(50))
updateMsg := &types.MsgStakeGateway{
// Prepare an updated gateway with a lower stake.
updatedStake := cosmostypes.NewCoin("upokt", math.NewInt(50))
updateMsg := &gatewaytypes.MsgStakeGateway{
Address: addr,
Stake: &updatedStake,
}

// Verify that it fails
// Verify that it fails.
_, err = srv.StakeGateway(ctx, updateMsg)
require.Error(t, err)

// Verify that the gateway stake is unchanged
// Verify that the gateway stake is unchanged.
gatewayFound, isGatewayFound := k.GetGateway(ctx, addr)
require.True(t, isGatewayFound)
require.Equal(t, initialStake.Amount, gatewayFound.Stake.Amount)
}

func TestMsgServer_StakeGateway_FailBelowMinStake(t *testing.T) {
k, ctx := keepertest.GatewayKeeper(t)
srv := keeper.NewMsgServerImpl(k)

addr := sample.AccAddress()
gatewayStake := cosmostypes.NewInt64Coin(volatile.DenomuPOKT, 100)
minStake := gatewayStake.AddAmount(math.NewInt(1))
expectedErr := gatewaytypes.ErrGatewayInvalidStake.Wrapf("gateway %q must stake at least %s", addr, minStake)

// Set the minimum stake to be greater than the gateway stake.
params := k.GetParams(ctx)
params.MinStake = &minStake
err := k.SetParams(ctx, params)
require.NoError(t, err)

// Prepare the gateway.
stakeMsg := &gatewaytypes.MsgStakeGateway{
Address: addr,
Stake: &gatewayStake,
}

// Attempt to stake the gateway & verify that the gateway does NOT exist.
_, err = srv.StakeGateway(ctx, stakeMsg)
require.ErrorContains(t, err, expectedErr.Error())
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I think that checking if ErrorContains(atewaytypes.ErrGatewayInvalidStake) is enough so you don't have to update the test if the error string message changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My personal preference is that the test be sensitive to the error message changing as this could be indicative of a couple things:

  1. Business logic changed such that a different error is returned for a given scenario. In many cases, this could be the same error type (e.g. ErrAppParamInvalid) which is wrapping (.Wrapf(...)) a message from a different error path.

    This could lead to false negatives (i.e. tests which pass but should not) appearing over time as the codebase evolves.

  2. The error or wrapped message was changed.

    In this case, I think we should want to have coverage over the error message such that we're confident that our expectation of how it looks is accurate. We're likely going to encounter these logs and error messages in bug reports, which need to be immediately useful.

_, isGatewayFound := k.GetGateway(ctx, addr)
require.False(t, isGatewayFound)
}
Loading