[Upgrades] Module upgrades and core param changes via authz txs #702
Conversation
okdas
added
protocol
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "grantee": "pokt1eeeksh2tvkh7wzmfrljnhw4wrhs55lcuvmekkw", | ||
| "authorization": { | ||
| "@type": "\/cosmos.authz.v1beta1.GenericAuthorization", | ||
| "msg": "\/cosmos.upgrade.v1beta1.MsgSoftwareUpgrade" |
There was a problem hiding this comment.
Is there a way we can leave a comment somehow or call it out? I know it's JSON so curious what ideas you have.
There was a problem hiding this comment.
@Olshansk with more grants introduced in #717 I think we need a separate document describing the available parameters and transactions.. There are too many. Separate small ticket?
Also, I've noticed some of our own parameters were removed but traces (tests, makefile target, etc) still present (for example min_relay_difficulty_bits - I assume there could be others). It can be tackled on the same ticket.
okdas
changed the title
WalkthroughThe recent updates introduce a significant restructuring of the application’s codebase by consolidating various keeper components into a unified Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@Olshansk can I get a preliminary review, please? I'm still thinking how/what can we test here. It will probably be a good idea to write a test that uses And we should be able to test core modules param changes via authz.. Looking into that now. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (5)
tools/scripts/upgrades/increase-block-size.sh (1)
12-12: Address the discussion comment.The discussion comment suggests considering authorization for changes to other core module parameters. This should be addressed to ensure comprehensive parameter management.
docusaurus/docs/protocol/upgrades/upgrade_list.md (1)
23-33: TestNet table is incomplete.The TestNet table in the
upgrade_list.mdfile is confirmed to be incomplete. The warning present in the file and the absence of a comprehensive list of TestNet upgrades in the repository support this conclusion.
- The table should be updated to include all recent TestNet upgrades.
Analysis chain
LGTM! Verify the completeness of the TestNet table.
The TestNet section is clear and correctly formatted.
However, ensure that the TestNet table is complete and accurate.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the completeness of the TestNet table. # Test: Check if there are additional protocol upgrades not listed in the table. # Expect: No additional upgrades found. # Assuming the upgrades are listed in a specific file or directory. if grep -q "v0.0.4" "docusaurus/docs/protocol/upgrades/testnet_upgrades.md"; then echo "TestNet table is complete." else echo "TestNet table is incomplete." fiLength of output: 224
Script:
#!/bin/bash # Search for all references to TestNet upgrades in the repository to verify the completeness of the table. rg -i "TestNet" --context 5Length of output: 40492
tools/scripts/upgrades/start-node.sh (1)
1-12: Paths and directories do not exist.The directories
/Users/dk/pocket/pokrtoll-for-releasesand/Users/dk/pocket/poktrolldo not exist. Please verify or update the paths and directories in the script to ensure they are correct.
/Users/dk/pocket/pokrtoll-for-releases/Users/dk/pocket/poktrollAnalysis chain
LGTM! Verify the paths and configurations.
The header and environment variables are clear and correctly formatted.
However, ensure that the paths and configurations are correct.
To further investigate the issue with the paths, we need to check if the directories exist and list their contents. This will help us determine if there are any files that might indicate a typo or a different file name.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the paths and configurations. # Test: Check if the paths to the binaries exist. # Expect: All paths should exist. if [ -f "/Users/dk/pocket/pokrtoll-for-releases/poktrolld" ] && [ -f "/Users/dk/pocket/poktroll/poktrolld" ]; then echo "Paths are correct." else echo "Paths are incorrect." fiLength of output: 112
Script:
#!/bin/bash # Description: Verify if the directories exist and list their contents. # Check if the directories exist if [ -d "/Users/dk/pocket/pokrtoll-for-releases" ]; then echo "Directory /Users/dk/pocket/pokrtoll-for-releases exists." echo "Contents of /Users/dk/pocket/pokrtoll-for-releases:" ls -l /Users/dk/pocket/pokrtoll-for-releases else echo "Directory /Users/dk/pocket/pokrtoll-for-releases does not exist." fi if [ -d "/Users/dk/pocket/poktroll" ]; then echo "Directory /Users/dk/pocket/poktroll exists." echo "Contents of /Users/dk/pocket/poktroll:" ls -l /Users/dk/pocket/poktroll else echo "Directory /Users/dk/pocket/poktroll does not exist." fiLength of output: 342
docusaurus/docs/operate/run_a_node/appgate_server.md (1)
28-28: Approved: Consistent reference update.The tip section has been correctly updated to reference
full_node_docker.md. Consider using "before" instead of "prior to" for conciseness.- prior to setting up a RelayMiner. + before setting up a RelayMiner.Tools
LanguageTool
[style] ~28-~28: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ploy a Full Node prior to setting up a RelayMiner. This ensures t...(EN_WORDINESS_PREMIUM_PRIOR_TO)
docusaurus/docs/protocol/upgrades/upgrade_procedure.md (1)
38-61: Address minor style issues.There are some minor style issues flagged by static analysis tools:
- The phrase "specific details" is redundant. Consider writing "details".
- Use a comma before "and" if it connects two independent clauses.
- The verb "does not support" should agree with the subject "DevNets".
- with specific details about the upgrade. + with details about the upgrade. - The height at which an upgrade should be executed and the node will be restarted. + The height at which an upgrade should be executed, and the node will be restarted. - DevNets currently do not support `cosmovisor`. + DevNets currently does not support `cosmovisor`.Tools
LanguageTool
[style] ~38-~38: This phrase is redundant. Consider writing “details”.
Context: ...upgrade/v1beta1/upgrade.proto#L14) with specific details about the upgrade. This information hel...(SPECIFIC_DETAILS)
[uncategorized] ~59-~59: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...t at which an upgrade should be executed and the node will be restarted. info: W...(COMMA_COMPOUND_SENTENCE_2)
Gitleaks
46-46: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
okdas
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
docusaurus/docs/protocol/upgrades/index.md (3)
8-14: Fix repeated verb in the table of contents.There's a repeated verb in the table of contents.
- - [Planned vs. Unplanned Upgrades](#planned-vs-unplanned-upgrades) - - [Breaking vs. Non-breaking Upgrades](#breaking-vs-non-breaking-upgrades) - - [Manual Interventions](#manual-interventions) + - [Planned vs. Unplanned Upgrades](#planned-vs-unplanned-upgrades) + - [Breaking vs. Non-breaking Upgrades](#breaking-vs-non-breaking-upgrades) + - [Manual Interventions](#manual-interventions)Tools
LanguageTool
[grammar] ~10-~10: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...#what-is-a-protocol-upgrade) - List of Upgrades - Upgrade Types - [Planned vs....(REPEATED_VERBS)
17-19: Consider adding more details about the DAO process and consensus-breaking changes.The section is clear but could benefit from more details about the DAO process and consensus-breaking changes to provide a better understanding.
- The Pocket Network is continuously evolving through regular protocol upgrades. We implement software upgrades via a DAO process, allowing validator nodes to incorporate consensus-breaking changes. These upgrades can be automatically applied when using [Cosmovisor](../../operate/run_a_node/full_node_cosmovisor.md), or manually if not using `cosmovisor`. + The Pocket Network is continuously evolving through regular protocol upgrades. We implement software upgrades via a DAO (Decentralized Autonomous Organization) process, which involves community voting and governance decisions, allowing validator nodes to incorporate consensus-breaking changes. These upgrades can be automatically applied when using [Cosmovisor](../../operate/run_a_node/full_node_cosmovisor.md), or manually if not using `cosmovisor`.
25-27: Consider adding a direct link to the upgrade list.The section is clear but could benefit from a direct link to the upgrade list in the documentation.
- While you can find a comprehensive list of [poktroll releases](https://github.com/pokt-network/poktroll/releases) on our GitHub, we also maintain a [list of upgrades](./upgrade_list.md) in our documentation. This list provides valuable information, including whether an upgrade involves breaking changes and if manual intervention is required from operators. + While you can find a comprehensive list of [poktroll releases](https://github.com/pokt-network/poktroll/releases) on our GitHub, we also maintain a [list of upgrades](./upgrade_list.md) in our documentation. This list provides valuable information, including whether an upgrade involves breaking changes and if manual intervention is required from operators. You can directly access the upgrade list [here](./upgrade_list.md).
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (3)
tools/scripts/upgrades/cosmovisor-start-node.sh (1)
2-5: Improve script documentation.The comments provide useful information, but consider adding more details about the purpose and usage of the script.
docusaurus/docs/protocol/upgrades/upgrade_procedure.md (2)
49-49: Refine the phrase for conciseness.The phrase "specific details" is redundant. Consider writing "details".
- with specific details about the upgrade. + with details about the upgrade.Tools
LanguageTool
[style] ~49-~49: This phrase is redundant. Consider writing “details”.
Context: ...upgrade/v1beta1/upgrade.proto#L14) with specific details about the upgrade. This information hel...(SPECIFIC_DETAILS)
70-70: Use a comma before "and" in compound sentences.Use a comma before "and" if it connects two independent clauses.
- The height at which an upgrade should be executed and the node will be restarted. + The height at which an upgrade should be executed, and the node will be restarted.Tools
LanguageTool
[uncategorized] ~70-~70: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...t at which an upgrade should be executed and the node will be restarted. info: W...(COMMA_COMPOUND_SENTENCE_2)
| "messages": [ | ||
| { | ||
| "@type": "/cosmos.upgrade.v1beta1.MsgSoftwareUpgrade", | ||
| "authority": "pokt10d07y265gmmuvt4z0w9aw880jnsr700j8yv32t", |
There was a problem hiding this comment.
Potential security issue: Detected a Generic API Key.
The value "pokt10d07y265gmmuvt4z0w9aw880jnsr700j8yv32t" appears to be a generic API key. Ensure this is not exposing sensitive information.
Tools
Gitleaks
57-57: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| name: | ||
| type: string | ||
| description: (Optional) Semantic human readable name for the service | ||
| title: >- | ||
| TODO_BETA: Name is currently unused but acts as a reminder | ||
| that an optional onchain representation of the service is | ||
| necessary | ||
| session_id: | ||
| type: string | ||
| description: A unique pseudoranom ID for this session | ||
| title: >- | ||
| NOTE: session_id can be derived from the above values using | ||
| on-chain but is included in the header for convenience | ||
| session_start_block_height: | ||
| type: string | ||
| format: int64 | ||
| title: The height at which this session started | ||
| session_end_block_height: | ||
| type: string | ||
| format: int64 | ||
| description: >- | ||
| Note that`session_end_block_height` is a derivative of (`start` + | ||
| `num_blocks_per_session`) | ||
|
|
||
| as goverened by on-chain params at the time of the session start. | ||
|
|
||
| It is stored as an additional field to simplofy business logic in | ||
| case | ||
|
|
||
| the number of blocks_per_session changes during the session. | ||
|
|
||
|
|
||
| The height at which this session ended, this is the last block of | ||
| the session | ||
| description: >- | ||
| SessionHeader is a lightweight header for a session that can be passed | ||
| around. | ||
|
|
||
| It is the minimal amount of data required to hydrate & retrieve all | ||
| data relevant to the session. | ||
| closest_merkle_proof: | ||
| type: string | ||
| format: byte | ||
| description: The serialized SMST proof from the `#ClosestProof()` method. | ||
| poktroll.session.SessionHeader: | ||
| type: object | ||
| properties: | ||
| application_address: | ||
| type: string | ||
| title: >- | ||
| The Bech32 address of the application using cosmos' ScalarDescriptor | ||
| to ensure deterministic encoding | ||
| service: | ||
| title: The service this session is for | ||
| type: object | ||
| properties: | ||
| id: | ||
| type: string | ||
| description: Unique identifier for the service | ||
| title: >- | ||
| For example, what if we want to request a session for a certain | ||
| service but with some additional configs that identify it? | ||
| name: | ||
| type: string | ||
| description: (Optional) Semantic human readable name for the service | ||
| title: >- | ||
| TODO_BETA: Name is currently unused but acts as a reminder that an | ||
| optional onchain representation of the service is necessary | ||
| session_id: | ||
| type: string | ||
| description: A unique pseudoranom ID for this session | ||
| title: >- | ||
| NOTE: session_id can be derived from the above values using on-chain | ||
| but is included in the header for convenience | ||
| session_start_block_height: | ||
| type: string | ||
| format: int64 | ||
| title: The height at which this session started | ||
| session_end_block_height: | ||
| type: string | ||
| format: int64 | ||
| description: >- | ||
| Note that`session_end_block_height` is a derivative of (`start` + | ||
| `num_blocks_per_session`) | ||
|
|
||
| as goverened by on-chain params at the time of the session start. | ||
|
|
||
| It is stored as an additional field to simplofy business logic in case | ||
|
|
||
| the number of blocks_per_session changes during the session. | ||
|
|
||
|
|
||
| The height at which this session ended, this is the last block of the | ||
| session | ||
| description: >- | ||
| SessionHeader is a lightweight header for a session that can be passed | ||
| around. | ||
|
|
||
| It is the minimal amount of data required to hydrate & retrieve all data | ||
| relevant to the session. |
There was a problem hiding this comment.
Fix duplication errors in object definitions.
There are duplication errors in the object definitions for poktroll.proof.Params, poktroll.proof.Proof, and poktroll.session.SessionHeader.
- poktroll.proof.Params:
- type: object
- properties:
- min_relay_difficulty_bits:
- type: string
- format: uint64
- description: >-
- min_relay_difficulty_bits is the minimum difficulty in bits for a
- relay to
- be included in a Merkle proof.
- proof_request_probability:
- type: number
- format: float
- description: >-
- proof_request_probability is the probability of a session requiring a
- proof
- if it's cost (i.e. compute unit consumption) is below the
- ProofRequirementThreshold.
- proof_requirement_threshold:
- type: string
- format: uint64
- description: >-
- proof_requirement_threshold is the session cost (i.e. compute unit
- consumption)
- threshold which asserts that a session MUST have a corresponding proof
- when its cost
- is equal to or above the threshold. This is in contrast to the this
- requirement
- being determined probabilistically via ProofRequestProbability.
- proof_missing_penalty:
- description: >-
- proof_missing_penalty is the number of tokens (uPOKT) which should be
- slashed from a supplier
- when a proof is required (either via proof_requirement_threshold or
- proof_missing_penalty)
- but is not provided.
- type: object
- properties:
- denom:
- type: string
- amount:
- type: string
- description: Params defines the parameters for the module.
- poktroll.proof.Proof:
- type: object
- properties:
- supplier_address:
- type: string
- session_header:
- type: object
- properties:
- application_address:
- type: string
- title: >-
- The Bech32 address of the application using cosmos'
- ScalarDescriptor to ensure deterministic encoding
- service:
- title: The service this session is for
- type: object
- properties:
- id:
- type: string
- description: Unique identifier for the service
- title: >-
- For example, what if we want to request a session for a
- certain service but with some additional configs that identify
- it?
- name:
- type: string
- description: (Optional) Semantic human readable name for the service
- title: >-
- TODO_BETA: Name is currently unused but acts as a reminder
- that an optional onchain representation of the service is
- necessary
- session_id:
- type: string
- description: A unique pseudoranom ID for this session
- title: >-
- NOTE: session_id can be derived from the above values using
- on-chain but is included in the header for convenience
- session_start_block_height:
- type: string
- format: int64
- title: The height at which this session started
- session_end_block_height:
- type: string
- format: int64
- description: >-
- Note that`session_end_block_height` is a derivative of (`start` +
- `num_blocks_per_session`)
- as goverened by on-chain params at the time of the session start.
- It is stored as an additional field to simplofy business logic in
- case
- the number of blocks_per_session changes during the session.
- description: >-
- SessionHeader is a lightweight header for a session that can be passed
- around.
- It is the minimal amount of data required to hydrate & retrieve all
- data relevant to the session.
- closest_merkle_proof:
- type: string
- format: byte
- description: The serialized SMST proof from the `#ClosestProof()` method.
- poktroll.session.SessionHeader:
- type: object
- properties:
- application_address:
- type: string
- title: >-
- The Bech32 address of the application using cosmos' ScalarDescriptor
- to ensure deterministic encoding
- service:
- title: The service this session is for
- type: object
- properties:
- id:
- type: string
- description: Unique identifier for the service
- title: >-
- For example, what if we want to request a session for a certain
- service but with some additional configs that identify it?
- name:
- type: string
- description: (Optional) Semantic human readable name for the service
- title: >-
- TODO_BETA: Name is currently unused but acts as a reminder that an
- optional onchain representation of the service is necessary
- session_id:
- type: string
- description: A unique pseudoranom ID for this session
- title: >-
- NOTE: session_id can be derived from the above values using on-chain
- but is included in the header for convenience
- session_start_block_height:
- type: string
- format: int64
- title: The height at which this session started
- session_end_block_height:
- type: string
- format: int64
- description: >-
- Note that`session_end_block_height` is a derivative of (`start` +
- `num_blocks_per_session`)
- as goverened by on-chain params at the time of the session start.
- It is stored as an additional field to simplofy business logic in case
- the number of blocks_per_session changes during the session.
- description: >-
- SessionHeader is a lightweight header for a session that can be passed
- around.
- It is the minimal amount of data required to hydrate & retrieve all data
- relevant to the session.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| poktroll.proof.Params: | |
| type: object | |
| properties: | |
| min_relay_difficulty_bits: | |
| type: string | |
| format: uint64 | |
| description: >- | |
| min_relay_difficulty_bits is the minimum difficulty in bits for a | |
| relay to | |
| be included in a Merkle proof. | |
| proof_request_probability: | |
| type: number | |
| format: float | |
| description: >- | |
| proof_request_probability is the probability of a session requiring a | |
| proof | |
| if it's cost (i.e. compute unit consumption) is below the | |
| ProofRequirementThreshold. | |
| proof_requirement_threshold: | |
| type: string | |
| format: uint64 | |
| description: >- | |
| proof_requirement_threshold is the session cost (i.e. compute unit | |
| consumption) | |
| threshold which asserts that a session MUST have a corresponding proof | |
| when its cost | |
| is equal to or above the threshold. This is in contrast to the this | |
| requirement | |
| being determined probabilistically via ProofRequestProbability. | |
| TODO_MAINNET: Consider renaming this to | |
| `proof_requirement_threshold_compute_units`. | |
| proof_missing_penalty: | |
| description: >- | |
| proof_missing_penalty is the number of tokens (uPOKT) which should be | |
| slashed from a supplier | |
| when a proof is required (either via proof_requirement_threshold or | |
| proof_missing_penalty) | |
| but is not provided. | |
| type: object | |
| properties: | |
| denom: | |
| type: string | |
| amount: | |
| type: string | |
| description: Params defines the parameters for the module. | |
| poktroll.proof.Proof: | |
| type: object | |
| properties: | |
| supplier_address: | |
| type: string | |
| session_header: | |
| type: object | |
| properties: | |
| application_address: | |
| type: string | |
| title: >- | |
| The Bech32 address of the application using cosmos' | |
| ScalarDescriptor to ensure deterministic encoding | |
| service: | |
| title: The service this session is for | |
| type: object | |
| properties: | |
| id: | |
| type: string | |
| description: Unique identifier for the service | |
| title: >- | |
| For example, what if we want to request a session for a | |
| certain service but with some additional configs that identify | |
| it? | |
| name: | |
| type: string | |
| description: (Optional) Semantic human readable name for the service | |
| title: >- | |
| TODO_BETA: Name is currently unused but acts as a reminder | |
| that an optional onchain representation of the service is | |
| necessary | |
| session_id: | |
| type: string | |
| description: A unique pseudoranom ID for this session | |
| title: >- | |
| NOTE: session_id can be derived from the above values using | |
| on-chain but is included in the header for convenience | |
| session_start_block_height: | |
| type: string | |
| format: int64 | |
| title: The height at which this session started | |
| session_end_block_height: | |
| type: string | |
| format: int64 | |
| description: >- | |
| Note that`session_end_block_height` is a derivative of (`start` + | |
| `num_blocks_per_session`) | |
| as goverened by on-chain params at the time of the session start. | |
| It is stored as an additional field to simplofy business logic in | |
| case | |
| the number of blocks_per_session changes during the session. | |
| The height at which this session ended, this is the last block of | |
| the session | |
| description: >- | |
| SessionHeader is a lightweight header for a session that can be passed | |
| around. | |
| It is the minimal amount of data required to hydrate & retrieve all | |
| data relevant to the session. | |
| closest_merkle_proof: | |
| type: string | |
| format: byte | |
| description: The serialized SMST proof from the `#ClosestProof()` method. | |
| poktroll.session.SessionHeader: | |
| type: object | |
| properties: | |
| application_address: | |
| type: string | |
| title: >- | |
| The Bech32 address of the application using cosmos' ScalarDescriptor | |
| to ensure deterministic encoding | |
| service: | |
| title: The service this session is for | |
| type: object | |
| properties: | |
| id: | |
| type: string | |
| description: Unique identifier for the service | |
| title: >- | |
| For example, what if we want to request a session for a certain | |
| service but with some additional configs that identify it? | |
| name: | |
| type: string | |
| description: (Optional) Semantic human readable name for the service | |
| title: >- | |
| TODO_BETA: Name is currently unused but acts as a reminder that an | |
| optional onchain representation of the service is necessary | |
| session_id: | |
| type: string | |
| description: A unique pseudoranom ID for this session | |
| title: >- | |
| NOTE: session_id can be derived from the above values using on-chain | |
| but is included in the header for convenience | |
| session_start_block_height: | |
| type: string | |
| format: int64 | |
| title: The height at which this session started | |
| session_end_block_height: | |
| type: string | |
| format: int64 | |
| description: >- | |
| Note that`session_end_block_height` is a derivative of (`start` + | |
| `num_blocks_per_session`) | |
| as goverened by on-chain params at the time of the session start. | |
| It is stored as an additional field to simplofy business logic in case | |
| the number of blocks_per_session changes during the session. | |
| The height at which this session ended, this is the last block of the | |
| session | |
| description: >- | |
| SessionHeader is a lightweight header for a session that can be passed | |
| around. | |
| It is the minimal amount of data required to hydrate & retrieve all data | |
| relevant to the session. |
Tools
yamllint
[error] 27174-27174: duplication of key "poktroll.proof.Params" in mapping
(key-duplicates)
[error] 27228-27228: duplication of key "poktroll.proof.Proof" in mapping
(key-duplicates)
[error] 27296-27296: duplication of key "poktroll.session.SessionHeader" in mapping
(key-duplicates)
| ProofKeeper proofmodulekeeper.Keeper | ||
| TokenomicsKeeper tokenomicsmodulekeeper.Keeper | ||
| SharedKeeper sharedmodulekeeper.Keeper | ||
| // this line is used by starport scaffolding # stargate/app/keeperDeclaration |
There was a problem hiding this comment.
Point #2 from this conversation: #702 (comment)
Need to add the line back and add a comment.
There was a problem hiding this comment.
Ah yes - I was sure I already moved the line. Thanks for catching this.
app/upgrades.go
Outdated
| "github.com/pokt-network/poktroll/app/upgrades" | ||
| ) | ||
|
|
||
| // allUpgrades includes all upgrades that have been created, but not necessarily submitted on-chain |
There was a problem hiding this comment.
GitHub releases might not always be listed in this (allUpgrades) slice.
#PUC why?
| @@ -0,0 +1,80 @@ | |||
| package keepers | |||
|
|
|||
| import ( | |||
There was a problem hiding this comment.
Per comment #1 in #702 (comment). Can you please:
- Explain why we have diverged from cosmos standards
- Link to this PR in the comment for reference
- Mention ignite might stop working.
There was a problem hiding this comment.
- Done.
- Done.
- Ignite scaffolding already needs manual adjustments due to our proto/package structure. This change doesn't break functionality; we just need to move the scaffolded keeper to a separate package.
| @@ -116,6 +116,78 @@ | |||
| }, | |||
| "expiration": "2500-01-01T00:00:00Z" | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Is this necessary? (Given the other PR that went in)
There was a problem hiding this comment.
No and I don't see it in GitHub changes. Do you?
|
|
||
| echo -e "\nPre-requisites:" |
There was a problem hiding this comment.
NIT: Is it not easier to just do 1 echo statement?
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 702) |
| @@ -0,0 +1,54 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Add error handling for the shebang line.
Consider adding set -e to exit the script if any command fails.
#!/bin/bash
+ set -eCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/bin/bash | |
| #!/bin/bash | |
| set -e |
| POKTROLLD_OLD_BINARY_PATH=$HOME/pocket/pokrtoll-for-releases/poktrolld | ||
| POKTROLLD_NEW_BINARY_PATH=$HOME/pocket/poktroll/poktrolld |
There was a problem hiding this comment.
Use absolute paths for binaries.
Using absolute paths for binaries can prevent issues related to relative paths.
export POKTROLLD_OLD_BINARY_PATH=$HOME/pocket/pokrtoll-for-releases/poktrolld
export POKTROLLD_NEW_BINARY_PATH=$HOME/pocket/poktroll/poktrolld
+ export POKTROLLD_OLD_BINARY_PATH=/absolute/path/to/pokrtoll-for-releases/poktrolld
+ export POKTROLLD_NEW_BINARY_PATH=/absolute/path/to/poktroll/poktrolldCommittable suggestion was skipped due to low confidence.
|
|
||
| read -p "Do you want to continue? (y/n): " answer | ||
| if [[ $answer != "y" ]]; then | ||
| echo "Script execution cancelled." |
There was a problem hiding this comment.
Fix misspelling in user confirmation step.
"Cancelled" is a misspelling of "canceled".
echo "Script execution cancelled."
+ echo "Script execution canceled."Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Script execution cancelled." | |
| echo "Script execution canceled." |
Tools
GitHub Check: misspell
[warning] 32-32: [misspell] tools/scripts/upgrades/cosmovisor-start-node.sh#L32
"cancelled" is a misspelling of "canceled"
| export DAEMON_HOME=$HOME/.poktroll # `localnet_regenesis` creates new genesis in this directory by default. | ||
|
|
||
| # Cleans up old upgrade binary and home dir. | ||
| rm -rf $DAEMON_HOME |
There was a problem hiding this comment.
Add error handling for directory removal.
Consider adding error handling for the rm -rf command to ensure the directory is removed successfully.
rm -rf $DAEMON_HOME
+ if [ $? -ne 0 ]; then
+ echo "Failed to remove $DAEMON_HOME"
+ exit 1
+ fiCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -rf $DAEMON_HOME | |
| rm -rf $DAEMON_HOME | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to remove $DAEMON_HOME" | |
| exit 1 | |
| fi |
| rm -rf $DAEMON_HOME | ||
|
|
||
| # Runs regenesis. | ||
| make localnet_regenesis |
There was a problem hiding this comment.
Add error handling for the make command.
Consider adding error handling for the make localnet_regenesis command to ensure it completes successfully.
make localnet_regenesis
+ if [ $? -ne 0 ]; then
+ echo "Failed to run localnet_regenesis"
+ exit 1
+ fiCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| make localnet_regenesis | |
| make localnet_regenesis | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to run localnet_regenesis" | |
| exit 1 | |
| fi |
| mkdir -p $DAEMON_HOME/cosmovisor/genesis/bin/ $DAEMON_HOME/cosmovisor/upgrades/$POKTROLLD_UPGRADE_PLAN_NAME/bin/ | ||
| cp -r $POKTROLLD_OLD_BINARY_PATH $DAEMON_HOME/cosmovisor/genesis/bin/poktrolld | ||
| cp -r $POKTROLLD_NEW_BINARY_PATH $DAEMON_HOME/cosmovisor/upgrades/$POKTROLLD_UPGRADE_PLAN_NAME/bin/poktrolld |
There was a problem hiding this comment.
Add error handling for directory creation and file copying.
Consider adding error handling for the mkdir and cp commands to ensure they complete successfully.
mkdir -p $DAEMON_HOME/cosmovisor/genesis/bin/ $DAEMON_HOME/cosmovisor/upgrades/$POKTROLLD_UPGRADE_PLAN_NAME/bin/
+ if [ $? -ne 0 ]; then
+ echo "Failed to create directories"
+ exit 1
+ fi
cp -r $POKTROLLD_OLD_BINARY_PATH $DAEMON_HOME/cosmovisor/genesis/bin/poktrolld
+ if [ $? -ne 0 ]; then
+ echo "Failed to copy old binary"
+ exit 1
+ fi
cp -r $POKTROLLD_NEW_BINARY_PATH $DAEMON_HOME/cosmovisor/upgrades/$POKTROLLD_UPGRADE_PLAN_NAME/bin/poktrolld
+ if [ $? -ne 0 ]; then
+ echo "Failed to copy new binary"
+ exit 1
+ fiCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mkdir -p $DAEMON_HOME/cosmovisor/genesis/bin/ $DAEMON_HOME/cosmovisor/upgrades/$POKTROLLD_UPGRADE_PLAN_NAME/bin/ | |
| cp -r $POKTROLLD_OLD_BINARY_PATH $DAEMON_HOME/cosmovisor/genesis/bin/poktrolld | |
| cp -r $POKTROLLD_NEW_BINARY_PATH $DAEMON_HOME/cosmovisor/upgrades/$POKTROLLD_UPGRADE_PLAN_NAME/bin/poktrolld | |
| mkdir -p $DAEMON_HOME/cosmovisor/genesis/bin/ $DAEMON_HOME/cosmovisor/upgrades/$POKTROLLD_UPGRADE_PLAN_NAME/bin/ | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to create directories" | |
| exit 1 | |
| fi | |
| cp -r $POKTROLLD_OLD_BINARY_PATH $DAEMON_HOME/cosmovisor/genesis/bin/poktrolld | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to copy old binary" | |
| exit 1 | |
| fi | |
| cp -r $POKTROLLD_NEW_BINARY_PATH $DAEMON_HOME/cosmovisor/upgrades/$POKTROLLD_UPGRADE_PLAN_NAME/bin/poktrolld | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to copy new binary" | |
| exit 1 | |
| fi |
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 702) |
|
While tests are failing now, they were green on the previous commit. No changes to the go code in the last commit. |
## Summary Follow-up PR to #702 to document protocol upgrades procedure. ## Issue - #702 - #625 ## Type of change Select one or more: - [ ] New feature, functionality or library - [ ] Bug fix - [ ] Code health or cleanup - [x] Documentation - [ ] Other (specify) ## Testing **Documentation changes** (only if making doc changes) - [ ] `make docusaurus_start`; only needed if you make doc changes **Local Testing** (only if making code changes) - [ ] **Unit Tests**: `make go_develop_and_test` - [ ] **LocalNet E2E Tests**: `make test_e2e` - See [quickstart guide](https://dev.poktroll.com/developer_guide/quickstart) for instructions **PR Testing** (only if making code changes) - [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR. - **THIS IS VERY EXPENSIVE**, so only do it after all the reviews are complete. - Optionally run `make trigger_ci` if you want to re-trigger tests without any code changes - If tests fail, try re-running failed tests only using the GitHub UI as shown [here](https://github.com/pokt-network/poktroll/assets/1892194/607984e9-0615-4569-9452-4c730190c1d2) ## Sanity Checklist - [ ] I have tested my changes using the available tooling - [ ] I have commented my code - [ ] I have performed a self-review of my own code; both comments & source code - [ ] I create and reference any new tickets, if applicable - [ ] I have left TODOs throughout the codebase, if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new category for "Upgrades" in the documentation to enhance navigation related to protocol upgrades. - Launched comprehensive documentation on the governance mechanisms for protocol parameter adjustments, detailing the DAO's role and examples of changes. - Added a structured page for past and upcoming protocol upgrades, improving user understanding of upgrade statuses. - Released a document outlining the protocol upgrade process, specifying steps and criteria for implementation. - **Documentation Updates** - Improved organization and clarity throughout the documentation by refining content structure and descriptions related to protocol upgrades. - Updated sidebar positions and references to ensure accurate navigation and resource linking for "Full Node" documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
## Summary Follow-up PR to #702 to document protocol upgrades procedure. ## Issue - #702 - #625 ## Type of change Select one or more: - [ ] New feature, functionality or library - [ ] Bug fix - [ ] Code health or cleanup - [x] Documentation - [ ] Other (specify) ## Testing **Documentation changes** (only if making doc changes) - [ ] `make docusaurus_start`; only needed if you make doc changes **Local Testing** (only if making code changes) - [ ] **Unit Tests**: `make go_develop_and_test` - [ ] **LocalNet E2E Tests**: `make test_e2e` - See [quickstart guide](https://dev.poktroll.com/developer_guide/quickstart) for instructions **PR Testing** (only if making code changes) - [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR. - **THIS IS VERY EXPENSIVE**, so only do it after all the reviews are complete. - Optionally run `make trigger_ci` if you want to re-trigger tests without any code changes - If tests fail, try re-running failed tests only using the GitHub UI as shown [here](https://github.com/pokt-network/poktroll/assets/1892194/607984e9-0615-4569-9452-4c730190c1d2) ## Sanity Checklist - [ ] I have tested my changes using the available tooling - [ ] I have commented my code - [ ] I have performed a self-review of my own code; both comments & source code - [ ] I create and reference any new tickets, if applicable - [ ] I have left TODOs throughout the codebase, if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new category for "Upgrades" in the documentation to enhance navigation related to protocol upgrades. - Launched comprehensive documentation on the governance mechanisms for protocol parameter adjustments, detailing the DAO's role and examples of changes. - Added a structured page for past and upcoming protocol upgrades, improving user understanding of upgrade statuses. - Released a document outlining the protocol upgrade process, specifying steps and criteria for implementation. - **Documentation Updates** - Improved organization and clarity throughout the documentation by refining content structure and descriptions related to protocol upgrades. - Updated sidebar positions and references to ensure accurate navigation and resource linking for "Full Node" documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Daniel Olshansky <[email protected]>
Summary
Introduces protocol upgrades (consensus-breaking and not) and documents the upgrade process.
Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
Keepersstructure for improved management of application components.Documentation
Bug Fixes / Improvements
Chores