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

feat(feeshare): Developer Incentives #144

Merged
merged 34 commits into from
Mar 29, 2023
Merged

feat(feeshare): Developer Incentives #144

merged 34 commits into from
Mar 29, 2023

Conversation

Reecepbcups
Copy link
Contributor

@Reecepbcups Reecepbcups commented Feb 23, 2023

This PR is in replace of #75 & builds on main properly

custom/auth/ante/ante.go Show resolved Hide resolved
custom/auth/ante/ante.go Outdated Show resolved Hide resolved
x/feeshare/ante/expected_keepers.go Show resolved Hide resolved
@ZaradarBH ZaradarBH added enhancement New feature or request out of scope work that is unapproved by the community, but still essential for the L1 team labels Feb 23, 2023
@ZaradarBH ZaradarBH added this to the Proposal 11168 milestone Feb 23, 2023
@ZaradarBH ZaradarBH self-assigned this Feb 23, 2023
custom/auth/ante/ante.go Outdated Show resolved Hide resolved
proto/terra/feeshare/v1beta1/genesis.proto Show resolved Hide resolved
proto/terra/feeshare/v1beta1/genesis.proto Show resolved Hide resolved
@nghuyenthevinh2000 nghuyenthevinh2000 changed the base branch from main to feeshare February 24, 2023 10:19
@nghuyenthevinh2000 nghuyenthevinh2000 changed the base branch from feeshare to main February 24, 2023 10:20
Copy link
Contributor

@ZaradarBH ZaradarBH left a comment

Choose a reason for hiding this comment

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

Great job m8 and gz on the lead dev role @ Juno :)

x/feeshare/ante/ante.go Show resolved Hide resolved
x/feeshare/client/cli/tx.go Outdated Show resolved Hide resolved
@fragwuerdig
Copy link
Collaborator

Do NOT merge this branch @ZaradarBH, @nghuyenthevinh2000, @Reecepbcups: Something is going on. I tried different environments to run this branch - failed. There is some runtime issue going on with this feature. The nodes wont start on genesis transactions.

@nghuyenthevinh2000
Copy link
Member

Do NOT merge this branch @ZaradarBH, @nghuyenthevinh2000, @Reecepbcups: Something is going on. I tried different environments to run this branch - failed. There is some runtime issue going on with this feature. The nodes wont start on genesis transactions.

thanks you so much for testing this out, what is the log?

@fragwuerdig
Copy link
Collaborator

fragwuerdig commented Mar 11, 2023

@Reecepbcups @ZaradarBH @nghuyenthevinh2000 @inon-man: In app.go beginning at line 522 we have the following snippet:

app.mm.SetOrderInitGenesis( capabilitytypes.ModuleName, authtypes.ModuleName, banktypes.ModuleName, distrtypes.ModuleName, stakingtypes.ModuleName, slashingtypes.ModuleName, govtypes.ModuleName, markettypes.ModuleName, oracletypes.ModuleName, treasurytypes.ModuleName, wasmtypes.ModuleName, authz.ModuleName, minttypes.ModuleName, crisistypes.ModuleName, ibchost.ModuleName, genutiltypes.ModuleName, evidencetypes.ModuleName, ibctransfertypes.ModuleName, feegrant.ModuleName, feesharetypes.ModuleName )

This must be replaced by the following snippet:

app.mm.SetOrderInitGenesis( feesharetypes.ModuleName, capabilitytypes.ModuleName, authtypes.ModuleName, banktypes.ModuleName, distrtypes.ModuleName, stakingtypes.ModuleName, slashingtypes.ModuleName, govtypes.ModuleName, markettypes.ModuleName, oracletypes.ModuleName, treasurytypes.ModuleName, wasmtypes.ModuleName, authz.ModuleName, minttypes.ModuleName, crisistypes.ModuleName, ibchost.ModuleName, genutiltypes.ModuleName, evidencetypes.ModuleName, ibctransfertypes.ModuleName, feegrant.ModuleName )

This moves the feesharetypes.ModuleName up in the SetOrderInitGenesis. After setting higher priority for genesis initialization of the feeshare module the node should start normally on genesis (tested it locally, produced some blocks).

It might very well be that the liveness test fails nevertheless. But then its not because the node fails to start up but rather because expanding the local testnet from 4 to 6 nodes introduced a bug with this test. I guess, we should work on it in another branch.

Btw: Do we need migration handler for this? On the mainnet the parameter space for this is not yet initialized, or is it?

Another question: Why is only one workflow (snyk test) running on this branch?

@fragwuerdig
Copy link
Collaborator

fragwuerdig commented Mar 11, 2023

thanks you so much for testing this out, what is the log?

@nghuyenthevinh2000: The nodes simply did not start up on genesis:

grafik

Resolved with:

#144 (comment)

@ZaradarBH
Copy link
Contributor

ZaradarBH commented Mar 12, 2023

Do NOT merge this branch @ZaradarBH, @nghuyenthevinh2000, @Reecepbcups: Something is going on. I tried different environments to run this branch - failed. There is some runtime issue going on with this feature. The nodes wont start on genesis transactions.

@Reecepbcups @ZaradarBH @nghuyenthevinh2000 @inon-man: In app.go beginning at line 522 we have the following snippet:

app.mm.SetOrderInitGenesis( capabilitytypes.ModuleName, authtypes.ModuleName, banktypes.ModuleName, distrtypes.ModuleName, stakingtypes.ModuleName, slashingtypes.ModuleName, govtypes.ModuleName, markettypes.ModuleName, oracletypes.ModuleName, treasurytypes.ModuleName, wasmtypes.ModuleName, authz.ModuleName, minttypes.ModuleName, crisistypes.ModuleName, ibchost.ModuleName, genutiltypes.ModuleName, evidencetypes.ModuleName, ibctransfertypes.ModuleName, feegrant.ModuleName, feesharetypes.ModuleName )

This must be replaced by the following snippet:

app.mm.SetOrderInitGenesis( feesharetypes.ModuleName, capabilitytypes.ModuleName, authtypes.ModuleName, banktypes.ModuleName, distrtypes.ModuleName, stakingtypes.ModuleName, slashingtypes.ModuleName, govtypes.ModuleName, markettypes.ModuleName, oracletypes.ModuleName, treasurytypes.ModuleName, wasmtypes.ModuleName, authz.ModuleName, minttypes.ModuleName, crisistypes.ModuleName, ibchost.ModuleName, genutiltypes.ModuleName, evidencetypes.ModuleName, ibctransfertypes.ModuleName, feegrant.ModuleName )

This moves the feesharetypes.ModuleName up in the SetOrderInitGenesis. After setting higher priority for genesis initialization of the feeshare module the node should start normally on genesis (tested it locally, produced some blocks).

It might very well be that the liveness test fails nevertheless. But then its not because the node fails to start up but rather because expanding the local testnet from 4 to 6 nodes introduced a bug with this test. I guess, we should work on it in another branch.

Btw: Do we need migration handler for this? On the mainnet the parameter space for this is not yet initialized, or is it?

Another question: Why is only one workflow (snyk test) running on this branch?

@fragwuerdig , nice find m8! I think the workflow issue might be because the PR branch belongs to an external org, it might be messing with the GH action configuration. Havnt looked into it.

FYI I created an issue for the liveliness bug so we can all look into that together next week. #171

@fragwuerdig
Copy link
Collaborator

@Reecepbcups I have another commit for this branch (merging main into this branch to resolve conflicts). I don't know exactly why I could push ed35c8c. But it's clear that I cannot push anymore... Do you changed any permissions on your forked repo? Can you allow me to push commits?

@Reecepbcups
Copy link
Contributor Author

Reecepbcups commented Mar 14, 2023

@fragwuerdig Thanks for this! I missed running a local instance I got so caught up in my Juno upgrade.

Can confirm the reordering works

I did not change any permissions no. I have allow edits by maintainers on and have not touched anything since this PR was opened. So unless this org changed permissions, not sure why this is the case

Copy link
Member

@nghuyenthevinh2000 nghuyenthevinh2000 left a comment

Choose a reason for hiding this comment

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

Code looks good and best to merge this after sdk 45 upgrade

@ZaradarBH
Copy link
Contributor

@Reecepbcups any chance you can find time to bring this branch up-2-date with main so we can merge the PR after the 2.0 release? :)

@ZaradarBH ZaradarBH merged commit 1e7936c into classic-terra:main Mar 29, 2023
inon-man added a commit that referenced this pull request Jun 17, 2023
nghuyenthevinh2000 added a commit that referenced this pull request Jun 19, 2023
Co-authored-by: nghuyenthevinh2000 <nghuyenthevinh@gmail.com>
Co-authored-by: Till Ziegler <tz@schoeneweide.tk>
Co-authored-by: vincent <vincent.ch.cn@gmail.com>
Co-authored-by: chengwenxi <22697326+chengwenxi@users.noreply.github.com>
Co-authored-by: alchemist-ti <134163224+alchemist-ti@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request out of scope work that is unapproved by the community, but still essential for the L1 team
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants