[Upgrades] v0.0.12 upgrade #1043

okdas · 2025-01-24T21:49:25Z

Summary

Adding an upgrade handler for v0.0.12

Type of change

Select one or more from the following:

New feature, functionality or library
Consensus breaking; add the consensus-breaking label if so. See [Infra] Automatically add the consensus-breaking label #791 for details
Bug fix
Code health or cleanup
Documentation
Other (specify)

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

github-actions · 2025-01-24T21:49:42Z

app/upgrades/v0.0.12.go

+//   - Before: v0.0.11
+//   - After: v0.0.12
+
+// TODO_IN_THIS_PR: WIP. Using this diff as a starting point: https://github.com/pokt-network/poktroll/compare/v0.0.11...feat/proof-endblocker


[linter-name (fail-on-found)] _{reported by reviewdog 🐶}
// TODO_IN_THIS_PR: WIP. Using this diff as a starting point: v0.0.11...feat/proof-endblocker

github-actions · 2025-01-24T21:49:42Z

app/upgrades/v0.0.12.go

+//   - After: v0.0.12
+
+// TODO_IN_THIS_PR: WIP. Using this diff as a starting point: https://github.com/pokt-network/poktroll/compare/v0.0.11...feat/proof-endblocker
+// TODO_IN_THIS_PR: Wait for https://github.com/pokt-network/poktroll/pull/1042


[linter-name (fail-on-found)] _{reported by reviewdog 🐶}
// TODO_IN_THIS_PR: Wait for #1042

github-actions · 2025-01-24T21:49:42Z

app/upgrades/v0.0.12.go

+				},
+				StakingFee: &cosmosTypes.Coin{
+					Denom: "upokt",
+					// TODO_IN_THIS_PR: 100upokt a good value?


[linter-name (fail-on-found)] _{reported by reviewdog 🐶}
// TODO_IN_THIS_PR: 100upokt a good value?

github-actions · 2025-01-24T21:49:42Z

app/upgrades/v0.0.12.go

+			}
+			logger.Info("Successfully updated supplier params", "new_params", supplierParams)
+
+			// TODO_IN_THIS_PR: RevSharePercent / DefaultRevSharePercent has been changed from float32 to uint64.


[linter-name (fail-on-found)] _{reported by reviewdog 🐶}
// TODO_IN_THIS_PR: RevSharePercent / DefaultRevSharePercent has been changed from float32 to uint64.

github-actions · 2025-01-24T21:49:42Z

app/upgrades/v0.0.12.go

+			// and maintain two versions of protobuf files IF we need to loop through existing suppliers and update their onchain
+			// data.
+
+			// TODO_IN_THIS_PR: Add service.params.target_num_relays.


[linter-name (fail-on-found)] _{reported by reviewdog 🐶}
// TODO_IN_THIS_PR: Add service.params.target_num_relays.

github-actions · 2025-01-24T21:49:42Z

app/upgrades/v0.0.12.go

+			// data.
+
+			// TODO_IN_THIS_PR: Add service.params.target_num_relays.
+			// TODO_IN_THIS_PR: Add tokenomics.params.global_inflation_per_claim.


[linter-name (fail-on-found)] _{reported by reviewdog 🐶}
// TODO_IN_THIS_PR: Add tokenomics.params.global_inflation_per_claim.

github-actions · 2025-01-30T00:55:13Z

app/upgrades/v0.0.12.go

+// protobuf field. As a result, we expect existing on-chain data to switch to default value.
+// Investigate the impact of this change on existing on-chain data.
+//
+// TODO_IN_THIS_PR: decide if we need a proper module migration.


[linter-name (fail-on-found)] _{reported by reviewdog 🐶}
// TODO_IN_THIS_PR: decide if we need a proper module migration.

red-0ne

LGTM! I feel we are starting to improve our migration plans 👏

red-0ne · 2025-01-31T02:19:43Z

app/upgrades/v0.0.12.go

+				},
+				StakingFee: &cosmosTypes.Coin{
+					Denom: "upokt",
+					// TODO_IN_THIS_PR: 100upokt a good value?


I think 100upokt or 1000upokt is a good value for now. We have the transaction fees that also protect from sibyl.

It'll all be revisited prior to mainnet. 100upokt LGTM

red-0ne · 2025-01-31T02:24:49Z

app/upgrades/v0.0.12.go

+					// Force all services to have a 100% revshare to the supplier.
+					// Not something we would do on a real mainnet, but it's a quick way to resolve the issue.
+					// Currently, we don't break any existing suppliers (as all of them have a 100% revshare to the supplier).
+					service.RevShare = []*sharedtypes.ServiceRevenueShare{


Ideally we would be able to loop over the old revshare map by unmashalling to the old proto type then create the new Supplier proto with the new values.

But since we don't have proto versioning and we marked the old (float32) value as reserved, this is the next best option offered to us.

Olshansk

@okdas Looks good!

Overall, I do want us to take this as an opportunity to create a good reference + example.

keep it clean
do things right
iterate on process / template

Things aren't "on fire" and it's a "simple migration", so now is the time!

Olshansk · 2025-01-31T15:18:56Z

app/upgrades.go

-	upgrades.Upgrade_0_0_4,
-	upgrades.Upgrade_0_0_10,
-	upgrades.Upgrade_0_0_11,
+	upgrades.Upgrade_0_0_12,


What do you think of commenting out previous upgrades (explaining they're complete on Alpha, Beta, Main, etc) so the code also works as history?

Olshansk · 2025-01-31T15:19:29Z

app/upgrades.go

@@ -11,9 +11,7 @@ import (
 // The chain upgrade can be scheduled AFTER the new version (with upgrade strategy implemented) is released,


Isn't there a certain .md you're supposed to update (for documentation purposes) when we do an upgrade?

https://dev.poktroll.com/protocol/upgrades/upgrade_list

Olshansk · 2025-01-31T15:20:09Z

app/upgrades/types.go

@@ -16,8 +16,14 @@ import (
 const (
 	// The default PNF/DAO address in the genesis file for Alpha TestNet. Used to create new authz authorizations.
 	AlphaTestNetPnfAddress = "pokt1r6ja6rz6rpae58njfrsgs5n5sp3r36r2q9j04h"
+
 	// Authority address. Defaults to gov module address. Used to create new authz authorizations.


Suggested change

// Authority address. Defaults to gov module address. Used to create new authz authorizations.

// Authority address. Defaults to x/gov module account address. Used to create new authz authorizations.

Olshansk · 2025-01-31T15:21:31Z

app/upgrades/types.go

+	// DEV_NOTE: Use `keepers.UpgradeKeeper.Authority(ctx, &upgradetypes.QueryAuthorityRequest{})` to query the authority
+	// address for the current network. Keeping this variable for historical upgrades.


Suggested change

// DEV_NOTE: Use `keepers.UpgradeKeeper.Authority(ctx, &upgradetypes.QueryAuthorityRequest{})` to query the authority

// address for the current network. Keeping this variable for historical upgrades.

// TECHDEBT: DO NOT use AlphaTestNetAuthorityAddress.

// Use `keepers.UpgradeKeeper.Authority(ctx, &upgradetypes.QueryAuthorityRequest{})` to query the authority address of the current Alpha Network.

// This hard-coded address is kept for record keeping historical purposes.

Olshansk · 2025-01-31T15:22:59Z

app/upgrades/types.go

 	AlphaTestNetAuthorityAddress = "pokt10d07y265gmmuvt4z0w9aw880jnsr700j8yv32t"
+
+	// The default PNF/DAO address in the genesis file for Beta TestNet. Used to create new authz authorizations.


Wanted to remind myself by double-asking.

This isn't a module address, right?

Olshansk · 2025-01-31T16:16:30Z

app/upgrades/v0.0.12.go

+			// Add tokenomics module `global_inflation_per_claim` parameter per `config.yml`.
+			// We use GetParams() as `DaoRewardAddress` is different between networks and we don't want to hardcode it.
+			tokenomicsParams := keepers.TokenomicsKeeper.GetParams(ctx)
+			tokenomicsParams.GlobalInflationPerClaim = 0.1


Wdyt of of making local constants for all of the magic numbers in this file?

That way someone can look at the top of the file and quickly see the new values we're changing/setting, and all the "ugly business logic" is hidden in the implementation.

Olshansk · 2025-01-31T16:25:13Z

app/upgrades/v0.0.12.go

+
+			err := applyNewParameters(ctx)
+			if err != nil {
+				logger.Error("Failed to apply new parameters", "error", err)


Thoughts on (here and elsewhere) adding the version upgrade a a (key,val) as well?

You can either have a local v0.0.12 const, or update the logger context, or both!

Olshansk · 2025-01-31T16:26:09Z

app/upgrades/v0.0.12.go

+				return vm, err
+			}
+
+			// Since we changed the type of RevSharePercent from float32 to uint64, we need to update all on-chain data.


Can we moved this into a local helper to scope / separate the changes from the "boilerplate"?

Olshansk · 2025-01-31T16:28:19Z

app/upgrades/v0.0.12.go

+				return vm, err
+			}
+
+			logger.Info("Successfully completed v0.0.12 upgrade handler")


Let's make v0.0.12 a local constant and do some string formatting here.

It'll avoid copy-pasta errors in the future.

Olshansk · 2025-01-31T16:29:11Z

app/upgrades/v0.0.12.go

+			logger.Info("Updating all suppliers to have a 100% revshare to the supplier", "num_suppliers", len(suppliers))
+			for _, supplier := range suppliers {
+				for _, service := range supplier.Services {
+					// Force all services to have a 100% revshare to the supplier.


Can you change this to read the prior state and use it for the new state/

I realize it's not a big deal here, but I want us to take this as an opportunity to do it right, and set a right example/reference in the future.

okdas added the protocol General core protocol related changes label Jan 24, 2025

okdas added this to the Beta TestNet Iteration milestone Jan 24, 2025

okdas self-assigned this Jan 24, 2025

github-actions bot reviewed Jan 24, 2025

View reviewed changes

github-actions bot reviewed Jan 30, 2025

View reviewed changes

v0.0.12

f187c35

okdas force-pushed the upgrade/prep-v0.0.12 branch from 5a3decc to f187c35 Compare January 30, 2025 23:49

set RevSharePercentage for all

adc3c4d

okdas changed the title ~~[Upgrades][WIP] prepare for v0.0.12~~ [Upgrades] v0.0.12 migration Jan 31, 2025

okdas changed the title ~~[Upgrades] v0.0.12 migration~~ [Upgrades] v0.0.12 upgrade Jan 31, 2025

okdas requested a review from Olshansk January 31, 2025 01:54

okdas marked this pull request as ready for review January 31, 2025 01:55

red-0ne approved these changes Jan 31, 2025

View reviewed changes

Olshansk requested changes Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Upgrades] v0.0.12 upgrade #1043

[Upgrades] v0.0.12 upgrade #1043

okdas commented Jan 24, 2025 •

edited

Loading

github-actions bot Jan 24, 2025

github-actions bot Jan 24, 2025

github-actions bot Jan 24, 2025

github-actions bot Jan 24, 2025

github-actions bot Jan 24, 2025

github-actions bot Jan 24, 2025

github-actions bot Jan 30, 2025

red-0ne left a comment

red-0ne Jan 31, 2025

Olshansk Jan 31, 2025

red-0ne Jan 31, 2025

Olshansk left a comment

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

		@@ -11,9 +11,7 @@ import (
		// The chain upgrade can be scheduled AFTER the new version (with upgrade strategy implemented) is released,

	// Authority address. Defaults to gov module address. Used to create new authz authorizations.
	// Authority address. Defaults to x/gov module account address. Used to create new authz authorizations.

		// DEV_NOTE: Use `keepers.UpgradeKeeper.Authority(ctx, &upgradetypes.QueryAuthorityRequest{})` to query the authority
		// address for the current network. Keeping this variable for historical upgrades.

-	// DEV_NOTE: Use `keepers.UpgradeKeeper.Authority(ctx, &upgradetypes.QueryAuthorityRequest{})` to query the authority
-	// address for the current network. Keeping this variable for historical upgrades.
+	// TECHDEBT: DO NOT use AlphaTestNetAuthorityAddress.
+	// Use `keepers.UpgradeKeeper.Authority(ctx, &upgradetypes.QueryAuthorityRequest{})` to query the authority address of the current Alpha Network.
+	// This hard-coded address is kept for record keeping historical purposes.

		AlphaTestNetAuthorityAddress = "pokt10d07y265gmmuvt4z0w9aw880jnsr700j8yv32t"

		// The default PNF/DAO address in the genesis file for Beta TestNet. Used to create new authz authorizations.

[Upgrades] v0.0.12 upgrade #1043

Are you sure you want to change the base?

[Upgrades] v0.0.12 upgrade #1043

Conversation

okdas commented Jan 24, 2025 • edited Loading

Summary

Type of change

Sanity Checklist

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot Jan 24, 2025

Choose a reason for hiding this comment

github-actions bot Jan 30, 2025

Choose a reason for hiding this comment

red-0ne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okdas commented Jan 24, 2025 •

edited

Loading