-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Upgrades] v0.0.12 upgrade #1043
Changes from 8 commits
f187c35
adc3c4d
fb373c2
147949f
dd6d1df
220eff6
f7a8636
b8965af
9c2ed24
32163df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,199 @@ | ||
package upgrades | ||
|
||
import ( | ||
"context" | ||
|
||
"cosmossdk.io/math" | ||
storetypes "cosmossdk.io/store/types" | ||
upgradetypes "cosmossdk.io/x/upgrade/types" | ||
cosmosTypes "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/cosmos/cosmos-sdk/types/module" | ||
"github.com/pokt-network/poktroll/app/keepers" | ||
sharedtypes "github.com/pokt-network/poktroll/x/shared/types" | ||
) | ||
|
||
const Upgrade_0_0_12_PlanName = "v0.0.12" | ||
|
||
// Upgrade_0_0_12 handles the upgrade to release `v0.0.12`. | ||
// This is planned to be issued on both Pocket Network's Shannon Alpha & Beta TestNets. | ||
var Upgrade_0_0_12 = Upgrade{ | ||
PlanName: Upgrade_0_0_12_PlanName, | ||
CreateUpgradeHandler: func(mm *module.Manager, | ||
keepers *keepers.Keepers, | ||
configurator module.Configurator, | ||
) upgradetypes.UpgradeHandler { | ||
// Parameter configurations aligned with repository config.yml specifications. | ||
// These values reflect the delta between v0.0.11 and the main branch as of #1043. | ||
// Reference: | ||
// - Comparison: https://github.com/pokt-network/poktroll/compare/v0.0.11..main | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be using That way this will be "evergreen" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TLDR: I want to use Complex answer, feel free to skip: We're supposed to create a release and use artifacts (binaries & images) produced by that release to upgrade validators, full nodes, seeds, and relayminers. The release can be created off any branch or any commit. It MUST include the upgrade plan code (introduced by this PR). Now, the code in the But we are a small team, and we can track that drift to changes to upgrade, if necessary, before the release. So I want to compare to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving conversation to discord to get this over the finish line. https://discord.com/channels/824324475256438814/1332065342746525778/1336434350660784239 |
||
// - Direct diff: `git diff v0.0.11..main -- config.yml` | ||
// | ||
// DEV_NOTE: These parameter updates are derived from config.yml in the root directory | ||
// of this repository, which serves as the source of truth for all parameter changes. | ||
const ( | ||
supplierStakingFee = 1000000 // uPOKT | ||
serviceTargetNumRelays = 100 // num relays | ||
tokenomicsGlobalInflationPerClaim = 0.1 // % of the claim amount | ||
) | ||
|
||
applyNewParameters := func(ctx context.Context) (err error) { | ||
logger := cosmosTypes.UnwrapSDKContext(ctx).Logger() | ||
logger.Info("Starting parameter updates", "upgrade_plan_name", Upgrade_0_0_12_PlanName) | ||
|
||
// Set supplier module staking_fee to 1000000upokt, in line with the config.yml in the repo. | ||
// Verify via: | ||
// $ poktrolld q supplier params --node=... | ||
supplierParams := keepers.SupplierKeeper.GetParams(ctx) | ||
supplierParams.MinStake = &cosmosTypes.Coin{ | ||
Denom: "upokt", | ||
Amount: math.NewInt(supplierStakingFee), | ||
} | ||
err = keepers.SupplierKeeper.SetParams(ctx, supplierParams) | ||
if err != nil { | ||
logger.Error("Failed to set supplier params", "error", err) | ||
return err | ||
} | ||
logger.Info("Successfully updated supplier params", "new_params", supplierParams) | ||
|
||
// Add service module `target_num_relays` parameter, in line with the config.yml in the repo. | ||
// Verify via: | ||
// $ poktrolld q service params --node=... | ||
serviceParams := keepers.ServiceKeeper.GetParams(ctx) | ||
serviceParams.TargetNumRelays = serviceTargetNumRelays | ||
err = keepers.ServiceKeeper.SetParams(ctx, serviceParams) | ||
if err != nil { | ||
logger.Error("Failed to set service params", "error", err) | ||
return err | ||
} | ||
logger.Info("Successfully updated service params", "new_params", serviceParams) | ||
|
||
// Add tokenomics module `global_inflation_per_claim` parameter, in line with the config.yml in the repo. | ||
// Verify via: | ||
// $ poktrolld q tokenomics params --node=... | ||
tokenomicsParams := keepers.TokenomicsKeeper.GetParams(ctx) | ||
tokenomicsParams.GlobalInflationPerClaim = tokenomicsGlobalInflationPerClaim | ||
err = keepers.TokenomicsKeeper.SetParams(ctx, tokenomicsParams) | ||
if err != nil { | ||
logger.Error("Failed to set tokenomics params", "error", err) | ||
return err | ||
} | ||
logger.Info("Successfully updated tokenomics params", "new_params", tokenomicsParams) | ||
return nil | ||
} | ||
|
||
// Helper function to update all suppliers' RevShare to 100%. | ||
// This is necessary to ensure that we have that value populated before suppliers are connected. | ||
// | ||
updateSuppliersRevShare := func(ctx context.Context) error { | ||
logger := cosmosTypes.UnwrapSDKContext(ctx).Logger() | ||
suppliers := keepers.SupplierKeeper.GetAllSuppliers(ctx) | ||
logger.Info("Updating (overriding) all suppliers to delegate 100% revenue share to the supplier's operator address", | ||
"num_suppliers", len(suppliers)) | ||
|
||
for _, supplier := range suppliers { | ||
for _, service := range supplier.Services { | ||
if len(service.RevShare) > 1 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You also need to check if the address is different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed the logic a bit. If more than 1 supplier: log and overwrite, |
||
// Warning: Overwriting existing revshare settings without preserving history. | ||
okdas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Note: While the canonical approach would be using Module Upgrade (docs.cosmos.network/v0.46/building-modules/upgrade) | ||
okdas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// to handle protobuf type changes (see: github.com/cosmos/cosmos-sdk/blob/v0.46.0-rc1/x/bank/migrations/v043/store.go#L50-L71), | ||
// we've opted for direct overwrite because: | ||
// 1. No active revenue shares are impacted at time of writing | ||
// 2. Additional protobuf and repo structure changes would be required for proper (though unnecessary) migration | ||
|
||
// Create a string representation of just the revenue share addresses | ||
revShareAddresses := "[" | ||
for i, rs := range service.RevShare { | ||
if i > 0 { | ||
revShareAddresses += "," | ||
} | ||
revShareAddresses += rs.Address | ||
} | ||
revShareAddresses += "]" | ||
okdas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
logger.Warn( | ||
"Overwriting existing revenue share configuration", | ||
Olshansk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"supplier_operator", supplier.OperatorAddress, | ||
"supplier_owner", supplier.OwnerAddress, | ||
"service", service.ServiceId, | ||
"previous_revshare_count", len(service.RevShare), | ||
Olshansk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"previous_revshare_addresses", revShareAddresses, | ||
) | ||
service.RevShare = []*sharedtypes.ServiceRevenueShare{ | ||
{ | ||
Address: supplier.OperatorAddress, | ||
RevSharePercentage: uint64(100), | ||
}, | ||
} | ||
} else if len(service.RevShare) == 1 { | ||
// If there is only one revshare setting, we can safely overwrite it (because it has 100% | ||
// revenue share), keeping the existing address. | ||
logger.Info("Updating supplier's revenue share configuration", | ||
"supplier_operator", supplier.OperatorAddress, | ||
"supplier_owner", supplier.OwnerAddress, | ||
"service", service.ServiceId, | ||
"previous_revshare_address", service.RevShare[0].Address, | ||
) | ||
currentRevShare := service.RevShare[0] | ||
service.RevShare = []*sharedtypes.ServiceRevenueShare{ | ||
{ | ||
Address: currentRevShare.Address, | ||
RevSharePercentage: uint64(100), | ||
}, | ||
} | ||
} else { | ||
logger.Warn("That shouldn't happen: no revenue share configuration found for supplier", | ||
"supplier_operator", supplier.OperatorAddress, | ||
"supplier_owner", supplier.OwnerAddress, | ||
"service", service.ServiceId, | ||
) | ||
} | ||
} | ||
keepers.SupplierKeeper.SetSupplier(ctx, supplier) | ||
logger.Info("Updated supplier", | ||
"supplier_operator", supplier.OperatorAddress, | ||
"supplier_owner", supplier.OwnerAddress) | ||
} | ||
return nil | ||
} | ||
|
||
// Returns the upgrade handler for v0.0.12 | ||
return func(ctx context.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) { | ||
logger := cosmosTypes.UnwrapSDKContext(ctx).Logger() | ||
logger.Info("Starting upgrade handler", "upgrade_plan_name", Upgrade_0_0_12_PlanName) | ||
|
||
logger.Info("Starting parameter updates section", "upgrade_plan_name", Upgrade_0_0_12_PlanName) | ||
// Update all governance parameter changes. | ||
// This includes adding params, removing params and changing values of existing params. | ||
err := applyNewParameters(ctx) | ||
okdas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
logger.Error("Failed to apply new parameters", | ||
"upgrade_plan_name", Upgrade_0_0_12_PlanName, | ||
"error", err) | ||
return vm, err | ||
} | ||
|
||
logger.Info("Starting supplier RevShare updates section", "upgrade_plan_name", Upgrade_0_0_12_PlanName) | ||
// Override all suppliers' RevShare to be 100% delegate to the supplier's operator address | ||
err = updateSuppliersRevShare(ctx) | ||
if err != nil { | ||
logger.Error("Failed to update suppliers RevShare", | ||
"upgrade_plan_name", Upgrade_0_0_12_PlanName, | ||
"error", err) | ||
return vm, err | ||
} | ||
|
||
logger.Info("Starting module migrations section", "upgrade_plan_name", Upgrade_0_0_12_PlanName) | ||
vm, err = mm.RunMigrations(ctx, configurator, vm) | ||
if err != nil { | ||
logger.Error("Failed to run migrations", | ||
"upgrade_plan_name", Upgrade_0_0_12_PlanName, | ||
"error", err) | ||
return vm, err | ||
} | ||
|
||
logger.Info("Successfully completed upgrade handler", "upgrade_plan_name", Upgrade_0_0_12_PlanName) | ||
return vm, nil | ||
} | ||
}, | ||
// No changes to the KVStore in this upgrade. | ||
StoreUpgrades: storetypes.StoreUpgrades{}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,7 @@ metrics: | |
addr: :9070 | ||
pocket_node: | ||
query_node_rpc_url: tcp://localhost:26657 | ||
query_node_grpc_url: tcp://localhost:36658 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity - when/why did we change this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh - I wanted to test if relayminer works when connected to the node running on localhost. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So confirming this should be left as is or no? It's just a little confusing (without comments) why some ports are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
query_node_grpc_url: tcp://localhost:9090 | ||
tx_node_rpc_url: tcp://localhost:26657 | ||
suppliers: | ||
- service_id: anvil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment of release, we don't know when the upgrade will happen. My thinking, this should be a separate PR. But! We can prepare some changes in advance - that's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it!
Wdyt of creating another PR (updating the docs that's a draft), and leave a TODO here.
var allUpgrades = []upgrades.Upgrade{ // v0.0.4 was the first upgrade we implemented and tested on network that is no longer used. // upgrades.Upgrade_0_0_4, // v0.0.10 was the first upgrade we implemented on Alpha TestNet. // upgrades.Upgrade_0_0_10, // v0.0.11 was the Alpha TestNet exclusive upgrade to bring it on par with Beta TestNet. // upgrades.Upgrade_0_0_11, // TODO(@okdas, #XX): Update the documentation once a block height has been selected. // Ref: https://dev.poktroll.com/protocol/upgrades/upgrade_list // v0.0.12 - the first upgrade going live on both Alpha and Beta TestNets. upgrades.Upgrade_0_0_12,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm: I saw your changes in
.md
AFTER writing the message above. Leaving it as a reference.Wdyt of adding the output of the CLI transaction to say "Please update docusaurus/docs/protocol/upgrades/upgrade_list.md` now that you've submitted the plan?
You know how they say "Go to where your users are", I'm trying to "Go where the dev's eyes are".
They're looking at the CLI when something says
!!!!!!!!!
:)