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

Jump to bottom

feat!: use x/governors instead of x/staking #179

Closed
wants to merge 10 commits into from
Closed

feat!: use x/governors instead of x/staking #179

wants to merge 10 commits into from

Conversation

mtsitrin
Copy link
Collaborator

Description

Closes #XXX

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.

PR review checkboxes:

I have...

  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Targeted PR against the correct branch
  • included the correct type prefix in the PR title
  • Linked to the GitHub issue with discussion and accepted design
  • Targets only one GitHub issue
  • Wrote unit and integration tests
  • Wrote relevant migration scripts if necessary
  • All CI checks have passed
  • Added relevant godoc comments
  • Updated the scripts for local run, e.g genesis_config_commands.sh if the PR changes parameters
  • Add an issue in the e2e-tests repo if necessary

SDK Checklist

  • Import/Export Genesis
  • Registered Invariants
  • Registered Events
  • Updated openapi.yaml
  • No usage of go map
  • No usage of time.Now()
  • Used fixed point arithmetic and not float arithmetic
  • Avoid panicking in Begin/End block as much as possible
  • No unexpected math Overflow
  • Used sendCoin and not SendCoins
  • Out-of-block compute is bounded
  • No serialized ID at the end of store keys
  • UInt to byte conversion should use BigEndian

Full security checklist here

----;

For Reviewer:

  • Confirmed the correct type prefix in the PR title
  • Reviewers assigned
  • Confirmed all author checklist items have been addressed

---;

After reviewer approval:

  • In case the PR targets the main branch, PR should not be squash merge in order to keep meaningful git history.
  • In case the PR targets a release branch, PR must be rebased.

@mtsitrin mtsitrin linked an issue Apr 25, 2024 that may be closed by this pull request
use x/governors instead of x/staking #177
Open
@mtsitrin mtsitrin marked this pull request as ready for review April 25, 2024 16:25
danwt
danwt previously approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@danwt danwt left a comment

Choose a reason for hiding this comment

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

approved but not a fan of commenting out code, at all
please just delete instead, and leave a comment with a link to a github commit containing the commments

Or write todos

app/app.go
Comment on lines +60 to +61
// "github.com/cosmos/cosmos-sdk/x/genutil"
// genutiltypes "github.com/cosmos/cosmos-sdk/x/genutil/types"
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 just delete instead of commenting
or if commenting out, needs a TODO: and an explanation

app/app.go
@@ -194,10 +195,10 @@ var (
ModuleBasics = module.NewBasicManager(
auth.AppModuleBasic{},
authzmodule.AppModuleBasic{},
genutil.AppModuleBasic{},
// genutil.AppModuleBasic{},
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto re commenting

app/app.go
Comment on lines +579 to +582
// genutil.NewAppModule(
// app.AccountKeeper, app.StakingKeeper, app.BaseApp.DeliverTx,
// encodingConfig.TxConfig,
// ),
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto re commenting

app/export.go
Comment on lines +35 to +36
// TODO: read validators from sequencer modules

Copy link
Contributor

Choose a reason for hiding this comment

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

why dont we do it in this PR?

@omritoptix
Copy link
Contributor

@mtsitrin e2e tests fails. possibly due to genesis changes.

@omritoptix
Copy link
Contributor

omritoptix commented Apr 25, 2024 •
edited
Loading

@mtsitrin this change breaks the relayer. when trying to create an ibc connection, getting the following error (while trying to query for the unbonding period though it's defined in the genesis). I assume we need to route the staking grpc queries to the governors module:

Error: error creating clients: failed to create client on dst chain{dymension_100-1}: failed to get trusting period for chain{rollappevm_9988-1}: failed to query unbonding period from ccvconsumer, staking & fallback : failed to make ccvconsumer params request: rpc error: code = Unknown desc = ccvconsumer: unknown subspace: unknown request: failed to make staking params request: rpc error: code = Unknown desc = staking: unknown subspace: unknown request : rpc error: code = Unknown desc = unknown query path: unknown request

Screenshot 2024-04-26 at 0 33 02

This is one of the relevant parts from the relayer code:

func (cc *PenumbraProvider) QueryStakingParams(ctx context.Context) (*stakingtypes.Params, error) {
    res, err := stakingtypes.NewQueryClient(cc).Params(ctx, &stakingtypes.QueryParamsRequest{})
    if err != nil {
        return nil, err
    }
    return &res.Params, nil
}

In order to reproduce simply follow the extended readme until the point of setup_ibc.sh which will fail.

@mtsitrin mtsitrin marked this pull request as draft April 26, 2024 11:23
@omritoptix
Copy link
Contributor

closing for now until we decided the direction with that

@omritoptix omritoptix closed this May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwt danwt danwt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

use x/governors instead of x/staking
6 participants
@mtsitrin @omritoptix @danwt @zale144 @hoangdv2429 @anhductn2001