feat: add migration for authorization list #2366
Conversation
kingpinXD
changed the title
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2366 +/- ##
========================================
Coverage 68.64% 68.65%
========================================
Files 304 305 +1
Lines 19153 19158 +5
========================================
+ Hits 13148 13152 +4
- Misses 5356 5357 +1
Partials 649 649
|
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (8)
You can disable this status message by setting the WalkthroughThe recent updates encompass enhancements to the Zeta Chain project, particularly focusing on the Changes
Poem
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 Configration File (
|
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito and
swift1337
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- Makefile (1 hunks)
- changelog.md (1 hunks)
- x/authority/keeper/migrator.go (1 hunks)
- x/authority/migrations/v2/migrate.go (1 hunks)
- x/authority/migrations/v2/migrate_test.go (1 hunks)
- x/authority/module.go (2 hunks)
- x/authority/types/authorization_list_test.go (1 hunks)
Additional context used
LanguageTool
changelog.md
[uncategorized] ~7-~7: Possible missing article found. (AI_HYDRA_LEO_MISSING_A)
Context: ...## Unreleased ### Breaking Changes * [List of the breaking changes can be found in...
[grammar] ~65-~65: It appears that a hyphen is missing in the plural noun “to-dos”? (TO_DO_HYPHEN)
Context: ...ub.com//pull/2329) - fix TODOs in rpc unit tests * [2342](https://gith...
[misspelling] ~79-~79: This word is normally spelled with a hyphen. (EN_COMPOUNDS_CHERRY_PICKED)
Context: ...//pull/2327) - partially cherry picked the fix to Bitcoin outbound dust amount...
[uncategorized] ~88-~88: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ... to be the github event for the release name which should be the version. Removed ma...
[uncategorized] ~88-~88: When ‘mac-specific’ is used as a modifier, it is usually spelled with a hyphen. (SPECIFIC_HYPHEN)
Context: ...me which should be the version. Removed mac specific build as the arm build should handle th...
[style] ~89-~89: Consider changing the order of words to improve your wording. (TO_NOT_VB)
Context: ...ocker build step for non release builds to not overwrite the github tag * [2192](https...
[grammar] ~105-~105: Did you mean “sending”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ... * Updating admin policies now requires to send a governance proposal executing the `Up...
[uncategorized] ~177-~177: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...n/node/pull/1899) - add empty test files so packages are included in coverage * [19...
[uncategorized] ~228-~228: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... Breaking Changes *zetaclientd start: now requires 2 inputs from stdin: hotke...
[grammar] ~265-~265: Did you mean “running”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ....com//pull/1584) - allow to run E2E tests on any networks * [1746](http...
[grammar] ~274-~274: The operating system from Apple is written “macOS”. (MAC_OS)
Context: ...nto dockerhub on release for ubuntu and macos * Adjusted the pipeline for building an...
[grammar] ~275-~275: The operating system from Apple is written “macOS”. (MAC_OS)
Context: ... building and pushing docker images for MacOS to install and run docker * Added docke...
[uncategorized] ~281-~281: When ‘start-up’ is used as a noun or modifier, it needs to be hyphenated. (VERB_NOUN_CONFUSION)
Context: ...he new docker image that facilitate the start up automation * Adjusted the docker pipeli...
[grammar] ~302-~302: Before the countable noun ‘to’ an article or a possessive pronoun is necessary. (IN_NN_CC_VBG)
Context: ...ssues/1610) - add pending outtx hash to tracker after monitoring for 10 minutes * [1656...
[uncategorized] ~367-~367: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...er params data have been moved to chain params described below. *GetCoreParams: Ren...
[uncategorized] ~368-~368: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...arams described below. *GetCoreParams: Renamed intoGetChainParams. `/zeta-c...
[uncategorized] ~452-~452: Possible wrong verb form detected. Did you mean “been” or “being”? (BE_WITH_WRONG_VERB_FORM)
Context: ... update an observer, this can be either be run a tombstoned observer/validator or ...
[grammar] ~492-~492: Did you mean “testing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...pgrade name, and allow download. allows to test release * [1153](https://github.com/zet...
[misspelling] ~500-~500: This word is normally spelled with a hyphen. (EN_COMPOUNDS_CHERRY_PICK)
Context: ...github.com//pull/1235) - cherry pick all hotfix from v10.0.x (zero-amount, p...
Markdownlint
changelog.md
222-222: Expected: asterisk; Actual: dash (MD004, ul-style)
Unordered list style
473-473: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
513-513: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
217-217: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
239-239: null (MD024, no-duplicate-heading)
Multiple headings with the same content
286-286: null (MD024, no-duplicate-heading)
Multiple headings with the same content
342-342: null (MD024, no-duplicate-heading)
Multiple headings with the same content
218-218: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
358-358: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
363-363: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
367-367: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
372-372: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
241-241: null (MD034, no-bare-urls)
Bare URL used
531-531: null (MD047, single-trailing-newline)
Files should end with a single newline character
Additional comments not posted (8)
x/authority/keeper/migrator.go (2)
14-18: Initialization ofMigratorlooks correct.The
NewMigratorfunction properly initializes theMigratorstruct with anauthorityKeeper.
21-24: Proper delegation of migration logic.The
Migrate1to2method correctly delegates the migration process to theMigrateStorefunction, maintaining a clean separation of concerns.x/authority/migrations/v2/migrate_test.go (1)
13-28: Comprehensive testing of the migration logic.The
TestMigrateStorefunction thoroughly tests the migration process, ensuring that the authorization list is set correctly and that no errors are encountered.x/authority/module.go (2)
125-128: Correct implementation of migration registration.The migration registration logic in
AppModuleuses a panic to fail fast on critical configuration errors, which is an appropriate strategy during module initialization.
153-153: Consensus version updated correctly.The
ConsensusVersionmethod now correctly reflects the updated module version, which is crucial for the migration logic to function properly.Makefile (1)
239-239: Update tozetanode-upgradetarget: Verify the version change.The
OLD_VERSIONargument in thezetanode-upgradeDocker build command has been updated from'release/v16'to'release/v17'. Ensure that this version update is intentional and compatible with other system components.Verification successful
Update to
zetanode-upgradetarget: Verify the version change.The
OLD_VERSIONargument in thezetanode-upgradeDocker build command has been updated from'release/v16'to'release/v17'. The search results confirm that'release/v17'is correctly referenced in theMakefile.
- Makefile: Line with
OLD_VERSION='release/v17'.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the old version 'release/v16' to ensure they have been updated or deprecated correctly. # Test: Search for the old version string. Expect: No occurrences of 'release/v16'. rg --type all 'release/v16'Length of output: 27
Script:
#!/bin/bash # Description: Check for any references to the new version 'release/v17' to ensure it is being used correctly. # Test: Search for the new version string. Expect: Occurrences of 'release/v17' in relevant files. rg --type all 'release/v17'Length of output: 153
x/authority/types/authorization_list_test.go (1)
212-216: New Test Case Added: "validate default authorizations list"The addition of this test case is a positive step towards ensuring the correctness of the default authorizations list. It checks that the
DefaultAuthorizationsList()function returns a list that passes validation without errors, which is crucial for system integrity.changelog.md (1)
31-31: Ensure consistency in PR reference formatting.The entry for the migration script addition is well-placed under "Features". However, for consistency with other entries, consider using a complete sentence and ensuring the PR link is formatted similarly to others (e.g., using markdown link syntax if applicable).
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- changelog.md (1 hunks)
Additional context used
LanguageTool
changelog.md
[grammar] ~65-~65: It appears that a hyphen is missing in the plural noun “to-dos”? (TO_DO_HYPHEN)
Context: ...ub.com//pull/2329) - fix TODOs in rpc unit tests * [2342](https://gith...
[misspelling] ~80-~80: This word is normally spelled with a hyphen. (EN_COMPOUNDS_CHERRY_PICKED)
Context: ...//pull/2327) - partially cherry picked the fix to Bitcoin outbound dust amount...
[uncategorized] ~89-~89: When ‘mac-specific’ is used as a modifier, it is usually spelled with a hyphen. (SPECIFIC_HYPHEN)
Context: ...me which should be the version. Removed mac specific build as the arm build should handle th...
[style] ~90-~90: Consider changing the order of words to improve your wording. (TO_NOT_VB)
Context: ...ocker build step for non release builds to not overwrite the github tag * [2192](https...
[grammar] ~106-~106: Did you mean “sending”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ... * Updating admin policies now requires to send a governance proposal executing the `Up...
[uncategorized] ~178-~178: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...n/node/pull/1899) - add empty test files so packages are included in coverage * [19...
[uncategorized] ~229-~229: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... Breaking Changes *zetaclientd start: now requires 2 inputs from stdin: hotke...
[grammar] ~266-~266: Did you mean “running”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ....com//pull/1584) - allow to run E2E tests on any networks * [1746](http...
[grammar] ~275-~275: The operating system from Apple is written “macOS”. (MAC_OS)
Context: ...nto dockerhub on release for ubuntu and macos * Adjusted the pipeline for building an...
[grammar] ~276-~276: The operating system from Apple is written “macOS”. (MAC_OS)
Context: ... building and pushing docker images for MacOS to install and run docker * Added docke...
[uncategorized] ~282-~282: When ‘start-up’ is used as a noun or modifier, it needs to be hyphenated. (VERB_NOUN_CONFUSION)
Context: ...he new docker image that facilitate the start up automation * Adjusted the docker pipeli...
[grammar] ~303-~303: Before the countable noun ‘to’ an article or a possessive pronoun is necessary. (IN_NN_CC_VBG)
Context: ...ssues/1610) - add pending outtx hash to tracker after monitoring for 10 minutes * [1656...
[uncategorized] ~369-~369: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...arams described below. *GetCoreParams: Renamed intoGetChainParams. `/zeta-c...
[uncategorized] ~373-~373: Possible missing article found. (AI_HYDRA_LEO_MISSING_AN)
Context: ...ver/getTssAddress/{bitcoin_chain_id}` . Optional bitcoin chain id can now be passed as a...
[uncategorized] ~373-~373: Possible missing article found. (AI_HYDRA_LEO_MISSING_THE)
Context: ... parameter to fetch the correct tss for required BTC chain. This parameter only affects ...
[uncategorized] ~453-~453: Possible wrong verb form detected. Did you mean “been” or “being”? (BE_WITH_WRONG_VERB_FORM)
Context: ... update an observer, this can be either be run a tombstoned observer/validator or ...
[grammar] ~493-~493: Did you mean “testing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...pgrade name, and allow download. allows to test release * [1153](https://github.com/zet...
[misspelling] ~501-~501: This word is normally spelled with a hyphen. (EN_COMPOUNDS_CHERRY_PICK)
Context: ...github.com//pull/1235) - cherry pick all hotfix from v10.0.x (zero-amount, p...
Markdownlint
changelog.md
223-223: Expected: asterisk; Actual: dash (MD004, ul-style)
Unordered list style
474-474: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
514-514: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
218-218: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
240-240: null (MD024, no-duplicate-heading)
Multiple headings with the same content
287-287: null (MD024, no-duplicate-heading)
Multiple headings with the same content
343-343: null (MD024, no-duplicate-heading)
Multiple headings with the same content
219-219: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
359-359: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
364-364: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
368-368: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
373-373: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
242-242: null (MD034, no-bare-urls)
Bare URL used
532-532: null (MD047, single-trailing-newline)
Files should end with a single newline character
Additional comments not posted (1)
changelog.md (1)
31-31: Documented the addition of the migration script clearly.The changelog entry for PR 2366 is appropriately documented under the "Features" section, which helps in tracking the changes made for adding the authorizations table. This is crucial for historical tracking and understanding the evolution of the project's features.
gartnera
force-pushed
the
authorizations-list-migration
branch
from
4803425 to
f4330ec
Compare
Description
Closes:#2311
Closes: #2376
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
ConsensusVersionto ensure proper versioning and avoid conflicts during migration.Tests
Documentation