refactor(x/authz): set environment in context #20502
Conversation
WalkthroughWalkthroughThe updates primarily involve transitioning context key handling from structured types to variable-based keys across various modules. This change simplifies the context value retrieval process and ensures consistency. Additionally, there are updates to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant CLI
participant Context
participant Environment
participant Logger
participant Viper
User->>CLI: Execute Command
CLI->>Context: Retrieve ViperContextKey
Context->>Viper: Get Viper Configuration
CLI->>Context: Retrieve LoggerContextKey
Context->>Logger: Get Logger Instance
CLI->>Environment: Set EnvironmentContextKey
Environment-->>CLI: Provide Environment Context
CLI-->>User: Command Execution Result
sequenceDiagram
participant Module
participant Context
participant Environment
participant Authorization
Module->>Context: Set EnvironmentContextKey
Context->>Environment: Include Environment in Context
Module->>Authorization: Call Accept with Context
Authorization->>Environment: Validate Using Environment
Authorization-->>Module: Return Validation Result
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 (
|
github-actions
bot
added
C:x/staking
C:CLI
C:x/bank
C:x/genutil
julienrbrt
changed the title
![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.yml
Review profile: CHILL
Files selected for processing (15)
- UPGRADING.md (1 hunks)
- client/cmd.go (3 hunks)
- core/context/context.go (1 hunks)
- core/context/server_context.go (1 hunks)
- server/util.go (1 hunks)
- simapp/simd/cmd/testnet_test.go (1 hunks)
- tests/e2e/genutil/export_test.go (2 hunks)
- x/authz/keeper/keeper.go (3 hunks)
- x/authz/keys.go (1 hunks)
- x/bank/types/send_authorization.go (2 hunks)
- x/genutil/client/cli/export_test.go (1 hunks)
- x/genutil/client/cli/genaccount_test.go (1 hunks)
- x/genutil/client/cli/init_test.go (7 hunks)
- x/genutil/client/testutil/helpers.go (1 hunks)
- x/staking/types/authz.go (3 hunks)
Files not reviewed due to errors (1)
- UPGRADING.md (no review received)
Files skipped from review due to trivial changes (3)
- core/context/context.go
- core/context/server_context.go
- x/authz/keys.go
Additional context used
Path-based instructions (12)
x/genutil/client/testutil/helpers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/simd/cmd/testnet_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/genutil/client/cli/genaccount_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/bank/types/send_authorization.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/e2e/genutil/export_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/staking/types/authz.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/genutil/client/cli/export_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/genutil/client/cli/init_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"client/cmd.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/authz/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/util.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
UPGRADING.md
[typographical] ~10-~10: It appears that a comma is missing.
Context: .... ## [Unreleased] ### SimApp In this section we describe the changes made in Cosmos ...
[typographical] ~10-~10: Unpaired symbol: ‘'’ seems to be missing
Context: ... describe the changes made in Cosmos SDK' SimApp. **These changes are directly ap...
[grammar] ~171-~171: The verb ‘depend’ can be stative. If ‘depending’ describes a state, change the sentence structure and use the base form of the verb.
Context: ...ild/docs/bsr/generated-sdks/go). If you were depending oncosmossdk.io/api/tendermint, pleas...
[grammar] ~181-~181: The verb ‘recommend’ is used with the gerund form.
Context: ...precation ofsdk.Context, we strongly recommend to use thecosmossdk.io/core/appmoduleinter...
[style] ~231-~231: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ved. It is required to migrate to v0.50 prior to upgrading to v0.51 for not missing any ...
[grammar] ~252-~252: The singular determiner ‘this’ may not agree with the plural noun ‘changes’. Did you mean “these”?
Context: ...Many functions have been removed due to this changes as the API can be smaller thank...
[uncategorized] ~252-~252: Possible missing comma found.
Context: ...functions have been removed due to this changes as the API can be smaller thanks to col...
[uncategorized] ~264-~264: Possible missing article found.
Context: ...cosmossdk.io/x/authz####x/bankBank was spun out into its owngo.mod. To ...
[uncategorized] ~274-~274: Possible missing article found.
Context: ...x/protocolpool module. ####x/groupGroup was spun out into its owngo.mod. To ...
[uncategorized] ~300-~300: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ams` A standalone Go module was created and it is accessible at "cosmossdk.io/x/par...
[uncategorized] ~335-~335: Possible missing comma found.
Context: ...o CometBFT (Part 2) The Cosmos SDK has migrated in its previous versions, to CometBFT. ...
[typographical] ~368-~368: It appears that a comma is missing.
Context: ...genesis.json` file. For existing chains this can be done in two ways: * During an u...
[typographical] ~370-~370: Consider adding a comma after this introductory phrase.
Context: ...s can be done in two ways: * During an upgrade the value is set in an upgrade handler....
[uncategorized] ~377-~377: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...here a catastrophic failure has occurred and the application should halt. However, t...
[misspelling] ~379-~379: Did you maybe mean “buy” or “be”?
Context: ... that returns an error, will gracefully by handled byBaseAppon behalf of the a...
[style] ~384-~384: Consider a shorter alternative to avoid wordiness.
Context: ...lizeBlock` is public and should be used in order to test and run operations. This means tha...
[grammar] ~397-~397: Did you mean “modifying”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ...begin blocker other modules, and allows to modify consensus parameters, and the changes a...
[uncategorized] ~435-~435: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...he case of successful msg(s) execution. Instead a new attribute is added to all message...
[grammar] ~462-~462: The word “clean-up” is a noun. The verb is spelled with a white space.
Context: ...s well as its settings. Useconfixto clean-up yourapp.toml. A nginx (or alike) rev...
[misspelling] ~466-~466: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... not supported anymore. To migrate from a unsupported database to a supported dat...
[typographical] ~466-~466: Consider adding a comma here.
Context: ...pported database to a supported database please use a database migration tool. ### Pro...
[typographical] ~483-~483: It appears that a comma is missing.
Context: ...tate-machine code. ### SimApp In this section we describe the changes made in Cosmos ...
[typographical] ~483-~483: Unpaired symbol: ‘'’ seems to be missing
Context: ... describe the changes made in Cosmos SDK' SimApp. **These changes are directly ap...
[uncategorized] ~547-~547: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...on. The global variable has been removed and the basic module manager can be now cre...
[uncategorized] ~583-~583: A comma may be missing after the conjunctive/linking adverb ‘Additionally’.
Context: ... not to be included in the binary. ::: AdditionallyAutoCLIautomatically adds the custom...
[grammar] ~614-~614: The preposition ‘to’ may be missing (allow someone to do something).
Context: ... a separate go.mod file which allows it be a standalone module. All the store impo...
[misspelling] ~635-~635: This word is normally spelled with a hyphen.
Context: ...available in the SDK that produces more human readable output, currently only available on Led...
[grammar] ~679-~679: Two determiners in a row. Choose either “the” or “a”.
Context: ...``` When usingdepinject/ `app v2`, the a tx config should be recreated from the ...
[grammar] ~703-~703: It looks like ‘interfaces’ doesn’t match ‘an’. Did you mean “an interface” or just “interfaces”?
Context: ...d ofsdk.Context. Any module that has an interfaces for them (like "expected keepers") will...
[uncategorized] ~727-~727: Possible missing comma found.
Context: ...EndBlock(context.Context) error ``` In case a module requires to return `abci.Valid...
[typographical] ~756-~756: Consider adding a comma here.
Context: ...ustom signer function. To find out more please read the [signer field](https://github....
[misspelling] ~769-~769: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...has migrated from a CometBFT genesis to a application managed genesis file. The g...
[misspelling] ~789-~789: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...ameter. An expedited proposal must have an higher voting threshold than a classic ...
[uncategorized] ~806-~806: Possible missing comma found.
Context: ... is extracted to have a separate go.mod file which allows it be a standalone module....
[grammar] ~806-~806: The preposition ‘to’ may be missing (allow someone to do something).
Context: ... a separate go.mod file which allows it be a standalone module. All the evidence i...
[uncategorized] ~813-~813: Possible missing comma found.
Context: ... is extracted to have a separate go.mod file which allows it to be a standalone modu...
[grammar] ~834-~834: Did you mean the possessive pronoun “its”?
Context: ...ing #### Rosetta Rosetta has moved to it's own [repo](https://github.com/cosmos/ro...
[uncategorized] ~835-~835: After the verb ‘interested’ the preposition “in” is used.
Context: ... by default. Any user who is interested on using the tool can connect it standalon...
[misspelling] ~836-~836: This word is normally spelled as one.
Context: ...de binary. The rosetta tool also allows multi chain connections. ## [v0.47.x](https://gith...
[typographical] ~858-~858: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...dom parameter changes during simulations, however, it does so through ParamChangeProposal ...
[grammar] ~860-~860: The noun should probably be in the singular form.
Context: ...teParamsgovernance proposals for each modules,AppModuleSimulationnow defines aA...
[style] ~875-~875: Consider using “formerly” to strengthen your wording.
Context: ... modules no longer support the REST API previously known as the LCD, and the `sdk.Msg#Rout...
[style] ~892-~892: Consider a shorter alternative to avoid wordiness.
Context: ...dd the following lines to yourapp.goin order to provide newer gRPC services: ```go aut...
[uncategorized] ~909-~909: Possible missing comma found.
Context: ...riod). These were unnecessary given as arguments as they were already present in theAp...
[uncategorized] ~913-~913: A comma may be missing after the conjunctive/linking adverb ‘Instead’.
Context: ...)was deprecated and has been removed. Instead you can use theTestEncodingConfig` fr...
[grammar] ~923-~923: The modal verb ‘must’ requires the verb’s base form.
Context: ... Replaces TheGoLevelDBversion must pinned to `v1.0.1-0.20210819022825-2ae1ddf74ef...
[grammar] ~934-~934: The word ‘replace’ is a verb. Did you mean the noun “replacement”?
Context: ...goproto. This allows you to remove the replace directivereplace github.com/gogo/prot...
[formatting] ~942-~942: You have used ‘I’ with an apostrophe, but either the verb is missing or you have not used the corresponding abbreviated form. Consider using one of the suggestions or removing the apostrophe.
Context: ...ly the rootproto/folder, set by theprotoc -Iflag). For example, assuming you put a...
[style] ~952-~952: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ace` annotations. We require them to be fully-scoped names. They will soon be used by code g...
[style] ~970-~970: The word “also” tends to be overused. Consider using a formal alternative to strengthen your wording.
Context: ...os.feegrant.v1beta1.FeeAllowanceI" ``` Please also check that in your own app's proto file...
[style] ~971-~971: Try using a more formal synonym for ‘check’.
Context: ...v1beta1.FeeAllowanceI" ``` Please also check that in your own app's proto files that...
[style] ~971-~971: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...otations. If so, then replace them with fully-qualified names, even though those names don't ac...
[locale-violation] ~1011-~1011: In American English, “take a look” is more commonly used.
Context: ...e modules keepers in previous versions. Have a look atsimapp.RegisterUpgradeHandlers()f...
[misspelling] ~1029-~1029: Did you mean “At a Time”, “At the Time”, or “At times”?
Context: ...x/gov##### Minimum Proposal Deposit At Time of Submission Thegovmodule has bee...
[misspelling] ~1036-~1036: Did you mean “at a time”, “at the time”, or “at times”?
Context: ...o utilize the minimum proposal deposits at time of submission, the migration logic need...
[uncategorized] ~1068-~1068: A comma is probably missing here.
Context: ...ng Tendermint consensus parameters. For migration it is required to call a specific migra...
[style] ~1095-~1095: Consider a shorter alternative to avoid wordiness.
Context: ...should still be imported in your app.go in order to handle this migration. Because the `x/...
[style] ~1157-~1157: Consider a shorter alternative to avoid wordiness.
Context: ...ally, new packages have been introduced in order to further split the codebase. Aliases are...
[uncategorized] ~1208-~1208: Possible missing article found.
Context: ...ur of each module housing and providing way to modify their parameters. Each module...
[typographical] ~1209-~1209: It appears that a comma is missing.
Context: ...aintained until April 18, 2023. At this point the module will reach end of life and b...
[style] ~1214-~1214: Consider a shorter alternative to avoid wordiness.
Context: ...the new implementation is calledv1. In order to submit a proposal with `submit-proposal...
[style] ~1221-~1221: ‘by accident’ might be wordy. Consider a shorter alternative.
Context: ...g delegations. Users that have unbonded by accident or wish to cancel a undelegation can no...
[misspelling] ~1221-~1221: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... unbonded by accident or wish to cancel a undelegation can now specify the amount...
[grammar] ~1225-~1225: The auxiliary verb ‘do’ requires the base form of the verb.
Context: ...v0.45.3/third_party/proto) now does not contains directly the [proto files](https://gith...
[style] ~1227-~1227: Consider a shorter alternative to avoid wordiness.
Context: ...build/cosmos/cosmos-sdk` as dependency, in order to avoid having to copy paste these files....
[grammar] ~1227-~1227: Did you mean “copy and paste”?
Context: ...dependency, in order to avoid having to copy paste these files. The protos can as well be...
[uncategorized] ~1245-~1245: Possible missing comma found.
Context: ..."]; } ``` When clients interact with a node they are required to set a codec in in ...
[duplication] ~1245-~1245: Possible typo: you repeated a word
Context: ...a node they are required to set a codec in in the grpc.Dial. More information can be ...
Markdownlint
UPGRADING.md
705-705: Expected: 2; Actual: 4
Unordered list indentation
706-706: Expected: 2; Actual: 4
Unordered list indentation
707-707: Expected: 2; Actual: 4
Unordered list indentation
708-708: Expected: 2; Actual: 4
Unordered list indentation
709-709: Expected: 2; Actual: 4
Unordered list indentation
710-710: Expected: 2; Actual: 4
Unordered list indentation
711-711: Expected: 2; Actual: 4
Unordered list indentation
712-712: Expected: 2; Actual: 4
Unordered list indentation
713-713: Expected: 2; Actual: 4
Unordered list indentation
51-51: Expected: 0 or 2; Actual: 1
Trailing spaces
185-185: Expected: 0 or 2; Actual: 1
Trailing spaces
636-636: Expected: 0 or 2; Actual: 1
Trailing spaces
84-84: Column: 1
Hard tabs
85-85: Column: 1
Hard tabs
86-86: Column: 1
Hard tabs
88-88: Column: 1
Hard tabs
89-89: Column: 1
Hard tabs
90-90: Column: 1
Hard tabs
91-91: Column: 1
Hard tabs
93-93: Column: 1
Hard tabs
94-94: Column: 1
Hard tabs
95-95: Column: 1
Hard tabs
96-96: Column: 1
Hard tabs
97-97: Column: 1
Hard tabs
102-102: Column: 1
Hard tabs
103-103: Column: 1
Hard tabs
104-104: Column: 1
Hard tabs
105-105: Column: 1
Hard tabs
106-106: Column: 1
Hard tabs
107-107: Column: 1
Hard tabs
108-108: Column: 1
Hard tabs
110-110: Column: 1
Hard tabs
111-111: Column: 1
Hard tabs
116-116: Column: 1
Hard tabs
117-117: Column: 1
Hard tabs
118-118: Column: 1
Hard tabs
119-119: Column: 1
Hard tabs
120-120: Column: 1
Hard tabs
121-121: Column: 1
Hard tabs
122-122: Column: 1
Hard tabs
123-123: Column: 1
Hard tabs
130-130: Column: 1
Hard tabs
131-131: Column: 1
Hard tabs
132-132: Column: 1
Hard tabs
134-134: Column: 1
Hard tabs
135-135: Column: 1
Hard tabs
136-136: Column: 1
Hard tabs
137-137: Column: 1
Hard tabs
139-139: Column: 1
Hard tabs
140-140: Column: 1
Hard tabs
141-141: Column: 1
Hard tabs
157-157: Column: 1
Hard tabs
162-162: Column: 3
Hard tabs
310-310: Column: 3
Hard tabs
311-311: Column: 2
Hard tabs
312-312: Column: 2
Hard tabs
313-313: Column: 2
Hard tabs
314-314: Column: 2
Hard tabs
315-315: Column: 2
Hard tabs
325-325: Column: 1
Hard tabs
326-326: Column: 1
Hard tabs
327-327: Column: 1
Hard tabs
404-404: Column: 2
Hard tabs
407-407: Column: 2
Hard tabs
408-408: Column: 1
Hard tabs
416-416: Column: 2
Hard tabs
420-420: Column: 2
Hard tabs
428-428: Column: 2
Hard tabs
529-529: Column: 1
Hard tabs
530-530: Column: 1
Hard tabs
531-531: Column: 1
Hard tabs
532-532: Column: 1
Hard tabs
533-533: Column: 2
Hard tabs
534-534: Column: 1
Hard tabs
553-553: Column: 1
Hard tabs
554-554: Column: 1
Hard tabs
555-555: Column: 1
Hard tabs
556-556: Column: 1
Hard tabs
557-557: Column: 1
Hard tabs
558-558: Column: 1
Hard tabs
559-559: Column: 1
Hard tabs
560-560: Column: 1
Hard tabs
561-561: Column: 1
Hard tabs
562-562: Column: 1
Hard tabs
573-573: Column: 1
Hard tabs
589-589: Column: 1
Hard tabs
596-596: Column: 1
Hard tabs
625-625: Column: 1
Hard tabs
645-645: Column: 1
Hard tabs
646-646: Column: 1
Hard tabs
647-647: Column: 1
Hard tabs
648-648: Column: 1
Hard tabs
649-649: Column: 1
Hard tabs
650-650: Column: 1
Hard tabs
651-651: Column: 1
Hard tabs
652-652: Column: 1
Hard tabs
653-653: Column: 1
Hard tabs
654-654: Column: 1
Hard tabs
655-655: Column: 1
Hard tabs
656-656: Column: 1
Hard tabs
657-657: Column: 1
Hard tabs
665-665: Column: 1
Hard tabs
666-666: Column: 1
Hard tabs
667-667: Column: 1
Hard tabs
668-668: Column: 1
Hard tabs
669-669: Column: 1
Hard tabs
670-670: Column: 1
Hard tabs
671-671: Column: 1
Hard tabs
672-672: Column: 1
Hard tabs
673-673: Column: 1
Hard tabs
674-674: Column: 1
Hard tabs
675-675: Column: 1
Hard tabs
676-676: Column: 1
Hard tabs
739-739: Column: 1
Hard tabs
740-740: Column: 1
Hard tabs
741-741: Column: 1
Hard tabs
743-743: Column: 1
Hard tabs
744-744: Column: 1
Hard tabs
745-745: Column: 1
Hard tabs
746-746: Column: 1
Hard tabs
993-993: Column: 1
Hard tabs
994-994: Column: 1
Hard tabs
995-995: Column: 1
Hard tabs
996-996: Column: 1
Hard tabs
997-997: Column: 1
Hard tabs
1021-1021: Column: 1
Hard tabs
1022-1022: Column: 1
Hard tabs
1023-1023: Column: 1
Hard tabs
1045-1045: Column: 1
Hard tabs
1046-1046: Column: 1
Hard tabs
1047-1047: Column: 1
Hard tabs
1048-1048: Column: 1
Hard tabs
1052-1052: Column: 1
Hard tabs
1053-1053: Column: 1
Hard tabs
1054-1054: Column: 1
Hard tabs
1055-1055: Column: 1
Hard tabs
1056-1056: Column: 1
Hard tabs
1057-1057: Column: 1
Hard tabs
1058-1058: Column: 1
Hard tabs
1059-1059: Column: 1
Hard tabs
1060-1060: Column: 1
Hard tabs
1076-1076: Column: 2
Hard tabs
1078-1078: Column: 2
Hard tabs
1079-1079: Column: 2
Hard tabs
1080-1080: Column: 2
Hard tabs
1081-1081: Column: 2
Hard tabs
1082-1082: Column: 2
Hard tabs
1083-1083: Column: 2
Hard tabs
1085-1085: Column: 1
Hard tabs
1087-1087: Column: 2
Hard tabs
1088-1088: Column: 2
Hard tabs
1089-1089: Column: 2
Hard tabs
1101-1101: Column: 1
Hard tabs
1102-1102: Column: 1
Hard tabs
1103-1103: Column: 1
Hard tabs
1177-1177: null
Spaces inside emphasis markers
1180-1180: null
Spaces inside emphasis markers
Additional comments not posted (12)
x/genutil/client/testutil/helpers.go (1)
38-39: The changes to use the new context key variables directly are correct and improve the efficiency of context management.simapp/simd/cmd/testnet_test.go (1)
71-72: The update to use the new context key variables directly in the test setup is correct and aligns with the broader refactoring goals of the PR.x/genutil/client/cli/genaccount_test.go (1)
95-96: The changes to use the new context key variables directly in the test setup are correct and align with the broader refactoring goals of the PR.x/bank/types/send_authorization.go (1)
7-8: The addition of new imports and the use ofEnvironmentContextKeyare necessary for the updated functionality of theSendAuthorizationtype. These changes align with the PR's objectives to manage environment contexts more effectively.Also applies to: 45-48
tests/e2e/genutil/export_test.go (1)
62-62: The update to use the new context key variable directly in the test setup is correct and aligns with the broader refactoring goals of the PR.x/staking/types/authz.go (1)
8-9: The addition of new imports and the use ofEnvironmentContextKeyare necessary for the updated functionality of theStakeAuthorizationtype. These changes align with the PR's objectives to manage environment contexts more effectively.Also applies to: 105-108
x/genutil/client/cli/export_test.go (1)
76-77: LGTM! The update from struct-based context keys to variable-based keys simplifies the context management.x/genutil/client/cli/init_test.go (1)
80-81: LGTM! Consistent update of context keys from struct-based to variable-based across various test functions enhances code maintainability.Also applies to: 114-115, 147-148, 175-176, 270-271, 320-321, 357-358
client/cmd.go (1)
382-382: LGTM! The update from struct-based context keys to variable-based keys in command context retrieval functions aligns with the broader refactoring goals.Also applies to: 391-391, 400-400
x/authz/keeper/keeper.go (2)
12-12: The import aliascorecontextforcosmossdk.io/core/contextis clear and avoids namespace conflicts, which is good practice in Go.
87-87: The use ofk.Environment.HeaderService.HeaderInfo(ctx).Timeto fetch the current time from the header service is a good encapsulation of environment-specific details.server/util.go (1)
230-231: The context is being enriched withViperandLoggerfromserverCtx. This is a good use of context to pass configuration and logging capabilities down the call stack.
| // pass the environment in the context | ||
| // users on server/v2 are expected to unwrap the environment from the context | ||
| // users on baseapp can still unwrap the sdk context | ||
| resp, err := authorization.Accept(context.WithValue(ctx, corecontext.EnvironmentContextKey, k.Environment), msg) |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results confirm that the only remaining instances of authorization.Accept using the old context are in the test file x/bank/types/send_authorization_test.go. These test cases need to be updated to reflect the new context requirement.
- Update the test cases in
x/bank/types/send_authorization_test.goto pass the modified context, similar to the change inx/authz/keeper/keeper.go.
Analysis chain
Passing the environment in the context to the authorization.Accept method is a significant change. Ensure that all consumers of this method are aware of the new context requirement and handle it appropriately.
Consider documenting this change prominently in the module's README or in the function's GoDoc to ensure that all developers are aware of the new context handling.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all calls to `authorization.Accept` correctly pass the modified context.
# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go $'authorization.Accept'
Length of output: 445
Script:
#!/bin/bash
# Description: Verify the context handling in `x/bank/types/send_authorization_test.go` to ensure it aligns with the new context requirement.
# Test: Display the lines around the calls to `authorization.Accept` in the test file.
rg --type go --context 5 $'authorization.Accept' x/bank/types/send_authorization_test.go
Length of output: 968
Script:
#!/bin/bash
# Description: Verify if there are any other test files or usage of `authorization.Accept` that need similar updates.
# Test: Search for the function usage across the entire codebase to ensure all instances are updated.
rg --type go $'authorization.Accept'
Length of output: 445
That was the original proposal yes. However then it leaks an authz dependency in staking and bank to get the context key. Same for feegrant :/ |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
x/staking/types/authz_test.go (1)
[!TIP]
Codebase VerificationThe initialization of
sdkCtxis found in the test filex/staking/types/authz_test.go:sdkCtx := testCtx.Ctx.WithHeaderInfo(coreheader.Info{})However, the
HeaderInfois being set with an emptycoreheader.Info{}. This might not reflect realistic test conditions as it does not provide specific height information.
- Ensure that
coreheader.Info{}is populated with realistic values, especially the height, to accurately reflect test conditions.Analysis chain
Line range hint
348-404: The test cases forcancel unbonding delegationare well-structured and cover various scenarios. Ensure that thesdkCtx.HeaderInfo().Heightis correctly mocked or set up to reflect realistic test conditions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct setup of sdkCtx.HeaderInfo().Height in the test environment. # Test: Search for the setup of sdkCtx in the test file. Expect: Proper initialization or mocking of HeaderInfo. rg --type go 'sdkCtx.HeaderInfo().Height' x/staking/types/authz_test.goLength of output: 73
Script:
#!/bin/bash # Description: Verify the initialization or mocking of sdkCtx in the test environment. # Test: Search for the initialization or mocking of sdkCtx in the test file. rg --type go 'sdkCtx' x/staking/types/authz_test.goLength of output: 943
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- x/bank/types/send_authorization_test.go (2 hunks)
- x/staking/types/authz_test.go (7 hunks)
Additional context used
Path-based instructions (2)
x/bank/types/send_authorization_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/staking/types/authz_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (6)
x/bank/types/send_authorization_test.go (3)
10-15: Imports have been updated to include new modules and context handling. Ensure these imports are necessary for the tests.
34-54: The mock implementations forheaderServiceandmockGasServiceare correctly set up to support the new context handling. This is crucial for ensuring that the tests reflect the new environment context integration.
58-63: The test setup correctly integrates the newEnvironmentContextKeyinto the context. This is essential for testing the behavior ofSendAuthorizationunder the new context handling system.x/staking/types/authz_test.go (3)
10-13: Imports have been updated to include new modules and context handling. Ensure these imports are necessary for the tests.
47-67: The mock implementations forheaderServiceandmockGasServiceare correctly set up to support the new context handling. This is crucial for ensuring that the tests reflect the new environment context integration.
72-77: The test setup correctly integrates the newEnvironmentContextKeyinto the context. This is essential for testing the behavior ofStakeAuthorizationunder the new context handling system.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- x/bank/types/send_authorization_test.go (2 hunks)
- x/staking/types/authz_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/bank/types/send_authorization_test.go
- x/staking/types/authz_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
x/bank/CHANGELOG.md (1)
Line range hint
35-35: Fix grammatical error in the changelog entry.- methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. + methods now return an error instead of panicking if any module accounts do not exist or are unauthorized.Tools
Markdownlint
42-42: Expected: 2; Actual: 4
Unordered list indentation
43-43: Expected: 2; Actual: 4
Unordered list indentation
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- x/authz/simulation/operations.go (3 hunks)
- x/bank/CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (2)
x/bank/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/authz/simulation/operations.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
LanguageTool
x/bank/CHANGELOG.md
[grammar] ~35-~35: The verb form ‘does’ does not seem to match the subject ‘accounts’.
Context: ...ead of panicking if any module accounts does not exist or unauthorized. * [#20517](h...
Markdownlint
x/bank/CHANGELOG.md
42-42: Expected: 2; Actual: 4
Unordered list indentation
43-43: Expected: 2; Actual: 4
Unordered list indentation
44-44: Expected: 2; Actual: 4
Unordered list indentation
45-45: Expected: 2; Actual: 4
Unordered list indentation
Additional comments not posted (3)
x/bank/CHANGELOG.md (1)
40-40: The changelog entry clearly communicates the changes made to theAcceptmethod in theAuthorizationinterface. This helps in understanding the impact of the changes on the API.x/authz/simulation/operations.go (2)
4-12: The imports are correctly updated to include necessary modules for context and environment handling as per the PR's objectives.
Line range hint
323-393: The modifications inSimulateMsgExecfunction correctly implement the new context handling mechanism. This is crucial for integrating the environment context in the authorization process. Ensure that theheaderServiceandmockGasServiceare correctly implemented as they play a key role in the new context setup.Verification successful
The implementations of
headerService,mockGasService, andmockGasMeterare correctly defined in the codebase, specifically in the filex/authz/simulation/operations.go. The methodsHeaderInfo,GasMeter, andConsumeare also properly implemented.
headerServiceis defined at line 373.mockGasServiceis defined at line 379.mockGasMeteris defined at line 387.- The corresponding methods are implemented correctly within the same file.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of headerService and mockGasService. # Test: Search for the implementation of these services. Expect: Proper implementation details. rg --type go $'headerService' 'mockGasService'Length of output: 98
Script:
#!/bin/bash # Description: Verify the implementation of headerService and mockGasService. # Search for the implementation of headerService ast-grep --lang go --pattern $'type headerService struct { $$$ } func (h headerService) HeaderInfo(ctx context.Context) coreheader.Info { $$$ }' # Search for the implementation of mockGasService ast-grep --lang go --pattern $'type mockGasService struct { coregas.Service } func (m mockGasService) GasMeter(ctx context.Context) coregas.Meter { $$$ }' # Search for the implementation of mockGasMeter ast-grep --lang go --pattern $'type mockGasMeter struct { coregas.Meter } func (m mockGasMeter) Consume(amount coregas.Gas, descriptor string) error { $$$ }'Length of output: 2118
Script:
#!/bin/bash # Description: Verify the implementation of headerService and mockGasService. # Search for the type definition of headerService ast-grep --lang go --pattern 'type headerService struct { $$$ }' # Search for the method implementation of HeaderInfo in headerService ast-grep --lang go --pattern 'func (h headerService) HeaderInfo(ctx context.Context) coreheader.Info { $$$ }' # Search for the type definition of mockGasService ast-grep --lang go --pattern 'type mockGasService struct { coregas.Service }' # Search for the method implementation of GasMeter in mockGasService ast-grep --lang go --pattern 'func (m mockGasService) GasMeter(ctx context.Context) coregas.Meter { $$$ }' # Search for the type definition of mockGasMeter ast-grep --lang go --pattern 'type mockGasMeter struct { coregas.Meter }' # Search for the method implementation of Consume in mockGasMeter ast-grep --lang go --pattern 'func (m mockGasMeter) Consume(amount coregas.Gas, descriptor string) error { $$$ }'Length of output: 3961
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
x/authz/CHANGELOG.md (1)
Line range hint
37-39: Correct the indentation of the unordered list to match Markdown standards.- * `NewMsgExec`, `NewMsgGrant` and `NewMsgRevoke` now takes strings as arguments instead of `sdk.AccAddress`. - * `ExportGenesis` also returns an error. - * `IterateGrants` returns an error, its handler function also returns an error. + * `NewMsgExec`, `NewMsgGrant` and `NewMsgRevoke` now takes strings as arguments instead of `sdk.AccAddress`. + * `ExportGenesis` also returns an error. + * `IterateGrants` returns an error, its handler function also returns an error.Tools
Markdownlint
37-37: Expected: 2; Actual: 4
Unordered list indentation
38-38: Expected: 2; Actual: 4
Unordered list indentation
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/authz/CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
x/authz/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
x/authz/CHANGELOG.md
37-37: Expected: 2; Actual: 4
Unordered list indentation
38-38: Expected: 2; Actual: 4
Unordered list indentation
39-39: Expected: 2; Actual: 4
Unordered list indentation
Additional comments not posted (1)
x/authz/CHANGELOG.md (1)
35-35: The changelog entry accurately reflects the changes made in PR #20502.
* main: docs: add docs on permissions (#20526) refactor(x/gov): set environment in context for legacy proposals (#20521) docs: migrate diagrams to mermaidjs (#20503) refactor(tools/hubl): don't use nil panic (#20515) refactor(x/authz): set environment in context (#20502) build(deps): Bump github.com/spf13/viper from 1.18.2 to 1.19.0 (#20519) feat(x/bank): Placing SendRestriction before Deduction of Coins in SendCoins (#20517) chore: sonar ignore directories with their own go.mods (#20509) ci: run action in merge queue (#20508)
* main: refactor(x/feegrant): set environment in context (#20529) refactor(x/accounts)!: accounts and auth module use the same account number tracking (#20405) chore: remove sonar from simapp (#20528) docs: add docs on permissions (#20526) refactor(x/gov): set environment in context for legacy proposals (#20521) docs: migrate diagrams to mermaidjs (#20503) refactor(tools/hubl): don't use nil panic (#20515) refactor(x/authz): set environment in context (#20502) build(deps): Bump github.com/spf13/viper from 1.18.2 to 1.19.0 (#20519) feat(x/bank): Placing SendRestriction before Deduction of Coins in SendCoins (#20517) chore: sonar ignore directories with their own go.mods (#20509) ci: run action in merge queue (#20508) refactor(server/v2/cometbft): update function comments (#20506)
Description
ref: #19640
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Documentation
UPGRADING.mdwith changes related to modules, transaction validators, and message validation for enhanced clarity.x/authz/CHANGELOG.mdabout theAcceptmethod now requiring the authz environment incontext.Context.Refactor
{}.Tests