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] Implement gateway unbonding period #1062

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Feb 10, 2025

Summary

This pull request introduces a Gateway unbonding period, preventing gateways from immediately unstaking after a MsgUnstakeGateway message is committed.

Primary Changes:

  • Updated the UnstakeGateway message handler to mark a gateway as unbonding instead of removing it.
  • Added EventGatewayUnbondingBegin and EventGatewayUnbondingEnd events.
  • Implemented the EndBlockerUnbondGateways keeper method to finalize gateways unbonding.

Secondary changes:

  • Introduced the GatewayUnbondingPeriodSessions shared governance parameter.
  • Updated unit and E2E tests to reflect these changes.

Issue

Type of change

Select one or more from the following:

Sanity Checklist

  • I have updated the GitHub Issue assignees, reviewers, labels, project, iteration and milestone
  • For docs, I have run make docusaurus_start
  • For code, I have run make go_develop_and_test and make test_e2e
  • For code, I have added the devnet-test-e2e label to run E2E tests in CI
  • For configurations, I have update the documentation
  • I added TODOs where applicable

@red-0ne red-0ne added gateway Changes related to the Gateway actor on-chain On-chain business logic labels Feb 10, 2025
@red-0ne red-0ne added this to the Beta TestNet Iteration milestone Feb 10, 2025
@red-0ne red-0ne self-assigned this Feb 10, 2025
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@red-0ne I apoligize for the "terse" and "short" comments in advance. My intention was to get through the whole PR at once.

Two major things I called out:

  1. A couple of verification tests
  2. Updating applications

Other than that, it's the usual nits / edits.

For the comments that are repetitive, feel free to resolve and consolidate the conversation in one thread.

Gateway gateway = 1 [(gogoproto.jsontag) = "gateway"];
// The end height of the session in which gateway was unstaked.
int64 session_end_height = 2 [(gogoproto.jsontag) = "session_end_height"];
// EventGatewayUnbondingBegin is emitted when a gateway begins unbonding.
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to add the consensus-breaking label for stuff like this.

cc @okdas as a reminder/need to automate this (ticket & PR are already in flight)

Comment on lines 16 to +19
string address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the gateway
cosmos.base.v1beta1.Coin stake = 2; // The total amount of uPOKT the gateway has staked
// Session end height at which the gateway initiated unstaking (0 if not unstaking)
uint64 unstake_session_end_height = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
string address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the gateway
cosmos.base.v1beta1.Coin stake = 2; // The total amount of uPOKT the gateway has staked
// Session end height at which the gateway initiated unstaking (0 if not unstaking)
uint64 unstake_session_end_height = 3;
// The Bech32 address of the gateway
string address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
// The total amount of uPOKT the gateway has staked
cosmos.base.v1beta1.Coin stake = 2;
// Session end height at which the gateway initiated unstaking (0 if not unstaking)
uint64 unstake_session_end_height = 3;

Copy link
Member

Choose a reason for hiding this comment

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

We're all learning whats easier to read

@@ -44,4 +44,9 @@ message Params {
// The amount of upokt that a compute unit should translate to when settling a session.
// DEV_NOTE: This used to be under x/tokenomics but has been moved here to avoid cyclic dependencies.
uint64 compute_units_to_tokens_multiplier = 9 [(gogoproto.jsontag) = "compute_units_to_tokens_multiplier", (gogoproto.moretags) = "yaml:\"compute_units_to_tokens_multiplier\""];
// gateway_unbonding_period_sessions is the number of sessions that a gateway must wait after
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// gateway_unbonding_period_sessions is the number of sessions that a gateway must wait after
// gateway_unbonding_period_sessions is the number of sessions that a gateway must wait after

Comment on lines +32 to +33
// EventGatewayUnbondingEnd is emitted when a gateway has completed
// unbonding.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// EventGatewayUnbondingEnd is emitted when a gateway has completed
// unbonding.
// EventGatewayUnbondingEnd is emitted when a gateway has completed unbonding.

// gateway_unbonding_period_sessions is the number of sessions that a gateway must wait after
// unstaking before their staked assets are moved to its account balance.
// Onchain business logic requires, and ensures, that the corresponding block count of the
// gateway unbonding period will exceed the end of its corresponding proof window close height.
Copy link
Member

Choose a reason for hiding this comment

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

Why are we talking about proof windows here?

)
if err != nil {
logger.Error(fmt.Sprintf(
"could not send %v coins from module %s to account %s due to %v",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"could not send %v coins from module %s to account %s due to %v",
"could not send %v coins from module %s to gateway account %s due to %v",

if err := k.UnbondGateway(ctx, &gateway); err != nil {
return err
}

Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to also update applications here?

Screenshot 2025-02-12 at 5 50 17 PM

"github.com/pokt-network/poktroll/x/gateway/keeper"
"github.com/pokt-network/poktroll/x/gateway/types"
sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
)

func TestMsgServer_UnstakeGateway_Success(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to see two more tests (use AI to help you):

  • func TestMsgServer_UnstakeGateway_DelegatingApplicationCheck_Success
  • func TestMsgServer_UnstakeGatewayAndRestake_DelegatingApplicationCheck_Success

Comment on lines +27 to +31
// IsActive returns whether the gateway is allowed to handle services at the
// given query height.
// A gateway that has not submitted an unstake message is always active.
// A gateway that has submitted an unstake message is active until the end of
// the session containing the height at which unstake message was submitted.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// IsActive returns whether the gateway is allowed to handle services at the
// given query height.
// A gateway that has not submitted an unstake message is always active.
// A gateway that has submitted an unstake message is active until the end of
// the session containing the height at which unstake message was submitted.
// IsActive returns whether the gateway is allowed to handle services at the given query height.
//
// Gateway activity rules:
// - Gateway without unstake message: Always active
// - Gateway with unstake message: Active until end of session containing unstake height

Comment on lines +64 to +67
// Mark the gateway as unstaking by recording the height at which it should
// no longer be able to process requests.
// The gateway MAY continue to process requests until the end of the current
// session. After that, the gateway will be considered inactive.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Mark the gateway as unstaking by recording the height at which it should
// no longer be able to process requests.
// The gateway MAY continue to process requests until the end of the current
// session. After that, the gateway will be considered inactive.
// Mark the gateway as unstaking by recording its deactivation height.
//
// Processing rules:
// - Gateway MAY continue processing requests until current session ends
// - After session end: Gateway becomes inactive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gateway Changes related to the Gateway actor on-chain On-chain business logic
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants