feat!: deprecate soft opt-out (backporting #1964) #1995
Conversation
github-actions
bot
added
C:Testing
tests/e2e/testlib/types.go
Outdated
| @@ -221,31 +221,31 @@ func (c *ChainState) UnmarshalJSON(data []byte) error { | |||
| func UnmarshalProposalWithType(inputMap json.RawMessage, proposalType string) (Proposal, error) { | |||
| var err error | |||
| switch proposalType { | |||
| case "main.TextProposal": | |||
| case "e2e.TextProposal": | |||
There was a problem hiding this comment.
If you were to run make test-trace you would get back:
Error reading test file 'tests/e2e/tracehandler_testdata/happyPath.json': e2e.ConsumerAdditionProposal is not a known proposal type
but make e2e-traces generated files with e2e.ConsumerAdditionProposal as their type.
There was a problem hiding this comment.
@p-offtermatt any idea about this?
There was a problem hiding this comment.
I don't quite get what you mean - make test-trace passes, right? (It passed in the CI on this very PR, and it also passes locally for me)
Is this still relevant? If yes, could you clarify?
There was a problem hiding this comment.
Maybe related: I fixed a failing unit test in tests/e2e/json_marshal_test.go
371cf97
There was a problem hiding this comment.
I don't quite get what you mean - make test-trace passes, right? (It passed in the CI on this very PR, and it also passes locally for me)
@p-offtermatt It didn't pass with those changes.
Maybe related: I fixed a failing unit test in tests/e2e/json_marshal_test.go
Thanks for the fix!
WalkthroughWalkthroughThe changes focus on the removal of the soft opt-out feature across various components, including code, documentation, and tests, and its replacement with a new mechanism called Partial Set Security (PSS). This deprecation impacts multiple files, especially those related to parameters, genesis settings, and test cases. The modifications also include updates to JSON structures for testing, ensuring backward compatibility, and cleaning up deprecated elements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConsumerApp
participant ConsumerModule
participant Validator
participant PartialSetSecurity
User->>ConsumerApp: Initiate validation request
ConsumerApp->>ConsumerModule: Process request
ConsumerModule->>Validator: Validate request
Validator->>PartialSetSecurity: Apply Partial Set Security
PartialSetSecurity-->>Validator: Return validation status
Validator-->>ConsumerModule: Validation result
ConsumerModule-->>ConsumerApp: Process result
ConsumerApp-->>User: Return final status
This sequence diagram illustrates the high-level interaction flow for validating a request under the new Partial Set Security mechanism. The User initiates a validation request, and the ConsumerApp processes it through the ConsumerModule. The Validator then applies the Partial Set Security, returns the status, and the final result is sent back to the User. Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional context usedPath-based instructions (1)
Additional comments not posted (1)
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 (
|
insumity
force-pushed
the
insumity/backport-1964
branch
2 times, most recently
from
33726af to
fd9ff79
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (6)
docs/docs/adrs/intro.md (2)
Line range hint
22-22: Suggested grammatical fix.Consider adding the missing article "a" for clarity.
- The spec is much more compressed and streamlined summary of everything as it is or should be. + The spec is a much more compressed and streamlined summary of everything as it is or should be.
Line range hint
26-26: Suggested grammatical fix.Consider adding a comma after "match" for better readability.
- Note the context/background should be written in the present tense. + Note the context/background should be written in the present tense,docs/docs/adrs/adr-009-soft-opt-out.md (1)
Line range hint
26-26: Grammar and Determiner Corrections:There are a couple of grammatical issues in the description of the function in the consumer beginblocker. Consider using 'run' instead of 'ran' and adding 'the' before 'smallest'.
-a function is ran which determines the so called _smallest non opt-out voting power_ +a function is run which determines the so-called _the smallest non opt-out voting power_Tools
LanguageTool
[duplication] ~13-~13: Possible typo: you repeated a word
Context: ...Change status to deprecated ## Status Deprecated Deprecated by [Partial Set Security](adr-015-parti...(ENGLISH_WORD_REPEAT_RULE)
[uncategorized] ~18-~18: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...needed to validate all consumer chains. Therefore a need exists to allow the bottomx%...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Markdownlint
16-16: Expected: 1; Actual: 0; Above
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
docs/docs/features/proposals.md (2)
Line range hint
105-105: Add a comma for clarity in conditional sentences.In the sentence, "Otherwise
top_Nwould be set to its default value of 0...", a comma after "Otherwise" would improve readability and clarity.- Otherwise `top_N` would be set to its default value of 0... + Otherwise, `top_N` would be set to its default value of 0...Tools
Markdownlint
80-80: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
Line range hint
12-12: Ensure proper formatting in markdown files.Several markdown linting issues have been identified, such as missing blank lines around headings and fenced code blocks. These formatting issues should be corrected to adhere to markdown standards and improve the readability of the documentation.
12 + 22 + 53 + 69 + 89 + 120 + 133 + 136 + 138 + 145 +Also applies to: 22-22, 53-53, 69-69, 89-89, 120-120, 133-133, 136-136, 138-138, 145-145
Tools
Markdownlint
80-80: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
x/ccv/consumer/module.go (1)
Line range hint
100-100: Consider refining the validation logic for clarity and maintainability.The validation function
ValidatePSSFeaturescould benefit from breaking down into smaller, more specific validation functions. This would improve readability and maintainability.+ // ValidateTopN checks if topN is within the specified range + func ValidateTopN(topN uint32) error { + if topN != 0 && (topN < 50 || topN > 100) { + return fmt.Errorf("Top N can either be 0 or in the range [50, 100]") + } + return nil + } + + // ValidateValidatorsPowerCap checks if validatorsPowerCap is within the specified range + func ValidateValidatorsPowerCap(validatorsPowerCap uint32) error { + if validatorsPowerCap != 0 && validatorsPowerCap > 100 { + return fmt.Errorf("validators' power cap has to be in the range [1, 100]") + } + return nil + } + func ValidatePSSFeatures(topN, validatorsPowerCap uint32) error { - if topN != 0 && (topN < 50 || topN > 100) { - return fmt.Errorf("Top N can either be 0 or in the range [50, 100]") - } - - if validatorsPowerCap != 0 && validatorsPowerCap > 100 { - return fmt.Errorf("validators' power cap has to be in the range [1, 100]") - } + err := ValidateTopN(topN) + if err != nil { + return err + } + + err = ValidateValidatorsPowerCap(validatorsPowerCap) + if err != nil { + return err + } return nil }
| "TopN": 100, | ||
| "ValidatorsPowerCap": 0, | ||
| "ValidatorSetCap": 0, | ||
| "Allowlist": null, | ||
| "Denylist": null |
There was a problem hiding this comment.
Ensure proper documentation and integration of new fields.
The introduction of fields like TopN, ValidatorsPowerCap, and others should be accompanied by updated documentation and thorough integration tests to verify their correct implementation.
Would you like assistance in updating the documentation or creating integration tests for these new fields?
tests/e2e/actions.go
Outdated
|
|
||
| consumerGenesis := ".app_state.ccvconsumer = " + tr.getConsumerGenesis(action.ProviderChain, action.ConsumerChain) | ||
| consumerGenesisChanges := tr.testConfig.chainConfigs[action.ConsumerChain].GenesisChanges | ||
|
|
||
| if consumerGenesisChanges != "" { | ||
| consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges + " | " + action.GenesisChanges | ||
| consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges | ||
| } | ||
| if action.GenesisChanges != "" { | ||
| consumerGenesis = consumerGenesis + " | " + action.GenesisChanges |
There was a problem hiding this comment.
Optimization suggestion for appending strings.
The method of appending strings for consumerGenesis can be optimized using a more efficient approach like strings.Builder or fmt.Sprintf for better performance and readability.
- consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges
- consumerGenesis = consumerGenesis + " | " + action.GenesisChanges
+ var builder strings.Builder
+ builder.WriteString(".app_state.ccvconsumer = ")
+ builder.WriteString(tr.getConsumerGenesis(action.ProviderChain, action.ConsumerChain))
+ if consumerGenesisChanges != "" {
+ builder.WriteString(" | ")
+ builder.WriteString(consumerGenesisChanges)
+ }
+ if action.GenesisChanges != "" {
+ builder.WriteString(" | ")
+ builder.WriteString(action.GenesisChanges)
+ }
+ consumerGenesis = builder.String()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.
| consumerGenesis := ".app_state.ccvconsumer = " + tr.getConsumerGenesis(action.ProviderChain, action.ConsumerChain) | |
| consumerGenesisChanges := tr.testConfig.chainConfigs[action.ConsumerChain].GenesisChanges | |
| if consumerGenesisChanges != "" { | |
| consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges + " | " + action.GenesisChanges | |
| consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges | |
| } | |
| if action.GenesisChanges != "" { | |
| consumerGenesis = consumerGenesis + " | " + action.GenesisChanges | |
| consumerGenesis := ".app_state.ccvconsumer = " + tr.getConsumerGenesis(action.ProviderChain, action.ConsumerChain) | |
| consumerGenesisChanges := tr.testConfig.chainConfigs[action.ConsumerChain].GenesisChanges | |
| var builder strings.Builder | |
| builder.WriteString(".app_state.ccvconsumer = ") | |
| builder.WriteString(tr.getConsumerGenesis(action.ProviderChain, action.ConsumerChain)) | |
| if consumerGenesisChanges != "" { | |
| builder.WriteString(" | ") | |
| builder.WriteString(consumerGenesisChanges) | |
| } | |
| if action.GenesisChanges != "" { | |
| builder.WriteString(" | ") | |
| builder.WriteString(action.GenesisChanges) | |
| } | |
| consumerGenesis = builder.String() |
| abci "github.com/cometbft/cometbft/abci/types" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" |
There was a problem hiding this comment.
Consider simplifying test setup.
The test setup involves a lot of boilerplate code. Consider extracting common setup tasks into helper functions to improve test readability and reduce redundancy.
- keeperParams := testkeeper.NewInMemKeeperParams(t)
- providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, keeperParams)
- providerKeeper.SetParams(ctx, providertypes.DefaultParams())
- ctx = ctx.WithBlockTime(tc.blockTime)
+ ctx, providerKeeper, ctrl, mocks := setupTestContext(t, tc.blockTime)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.
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| sdk "github.com/cosmos/cosmos-sdk/types" | |
| ctx, providerKeeper, ctrl, mocks := setupTestContext(t, tc.blockTime) |
Tip
Codebase Verification
Ensure proper error handling in test cases.
The test file contains generic error assertions such as require.Error(t, err) and require.NoError(t, err). To avoid false positives, it is recommended to assert specific error types or messages.
x/ccv/provider/keeper/proposal_test.go
Analysis chain
Ensure proper error handling in test cases.
Some test cases do not assert the specific type or message of the error, which could lead to false positives if the error is not the expected one.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that error handling in tests checks for specific errors.
# Test: Search for generic error assertions. Expect: Specific error checks.
rg --type go 'require.Error(t, err)' 'x/ccv/provider/keeper/proposal_test.go'
Length of output: 77
Script:
#!/bin/bash
# Description: Verify that error handling in tests checks for specific errors.
# Test: Search for generic error assertions and other potential error handling patterns. Expect: Specific error checks.
rg --type go 'require\.(Error|NoError|EqualError|ErrorContains)\(t, err' x/ccv/provider/keeper/proposal_test.go
Length of output: 371
tests/e2e/actions.go
Outdated
| @@ -2040,7 +2047,7 @@ | |||
| if err != nil { | |||
| log.Fatal(err, "\n", string(bz)) | |||
| } | |||
| tr.waitBlocks("provi", 10, 2*time.Minute) | |||
| tr.waitBlocks("provi", 20, 4*time.Minute) | |||
There was a problem hiding this comment.
Ensure proper error handling in invokeDoublesignSlash.
The function invokeDoublesignSlash lacks proper error handling for the scenario where useCometMock is false. It's crucial to handle potential errors from external commands to prevent crashes and ensure robustness.
- tr.waitBlocks("provi", 20, 4*time.Minute)
+ bz, err := tr.waitBlocks("provi", 20, 4*time.Minute)
+ if err != nil {
+ log.Fatal(err, "\n", string(bz))
+ }Committable suggestion was skipped due to low confidence.
tests/e2e/actions.go
Outdated
| consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges | ||
| } | ||
| if action.GenesisChanges != "" { | ||
| consumerGenesis = consumerGenesis + " | " + action.GenesisChanges |
There was a problem hiding this comment.
Optimize string operations in changeoverChain.
The string concatenation in changeoverChain can be improved for better performance and maintainability using strings.Builder or fmt.Sprintf.
- consumerGenesis = consumerGenesis + " | " + consumerGenesisChanges
- consumerGenesis = consumerGenesis + " | " + action.GenesisChanges
+ var builder strings.Builder
+ builder.WriteString(".app_state.ccvconsumer = ")
+ builder.WriteString(string(bz)) // Assuming `bz` holds the initial consumer genesis data.
+ if consumerGenesisChanges != "" {
+ builder.WriteString(" | ")
+ builder.WriteString(consumerGenesisChanges)
+ }
+ if action.GenesisChanges != "" {
+ builder.WriteString(" | ")
+ builder.WriteString(action.GenesisChanges)
+ }
+ consumerGenesis = builder.String()Committable suggestion was skipped due to low confidence.
tests/e2e/testlib/types.go
Outdated
| @@ -221,31 +221,31 @@ func (c *ChainState) UnmarshalJSON(data []byte) error { | |||
| func UnmarshalProposalWithType(inputMap json.RawMessage, proposalType string) (Proposal, error) { | |||
| var err error | |||
| switch proposalType { | |||
| case "main.TextProposal": | |||
| case "e2e.TextProposal": | |||
There was a problem hiding this comment.
Ensure proper error handling in UnmarshalProposalWithType.
The error handling in the UnmarshalProposalWithType function could be improved by directly returning the error from the json.Unmarshal function instead of checking if err == nil.
case "e2e.TextProposal":
prop := TextProposal{}
- err := json.Unmarshal(inputMap, &prop)
- if err == nil {
- return prop, nil
- }
+ if err := json.Unmarshal(inputMap, &prop); err != nil {
+ return nil, err
+ }
return prop, nilCommittable suggestion was skipped due to low confidence.
| Deprecated | ||
| Deprecated by [Partial Set Security](adr-015-partial-set-security.md) |
There was a problem hiding this comment.
Clarify ADR Deprecation:
The document states "Deprecated" twice which might be a typographical error. Consider revising for clarity.
-Deprecated
+Deprecated by [Partial Set Security](adr-015-partial-set-security.md)Committable suggestion was skipped due to low confidence.
| @@ -28,6 +28,7 @@ | |||
| "ValPowers": null, | |||
| "StakedTokens": null, | |||
| "IBCTransferParams": null, | |||
| "Params": null, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistent JSON structure found in consumer-misbehaviour.json.
The JSON structure in the file tests/e2e/tracehandler_testdata/consumer-misbehaviour.json is inconsistent. Specifically, the structure contains a boolean value where an object with keys is expected, leading to an error during validation.
- File:
tests/e2e/tracehandler_testdata/consumer-misbehaviour.json - Issue: Inconsistent JSON structure due to a boolean value.
- Lines: The error was detected around line 473.
Please review and correct the JSON structure for consistency and completeness.
Analysis chain
Verify the consistency and completeness of the JSON structure.
Ensure that the JSON structure in consumer-misbehaviour.json is consistent and complete for all scenarios outlined. This includes checking for any missing fields or incorrect data types that could affect test outcomes.
Also applies to: 57-61, 73-73, 91-93, 116-116, 133-140, 165-165, 185-185, 203-203, 214-214, 234-234, 283-283, 287-287, 307-307, 333-333, 337-337, 376-376, 380-380, 403-403, 430-430, 434-434, 457-457
We need to create a script that will actually verify the JSON structure for consistency and completeness. This can involve using tools like jq to parse and validate the JSON data.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency and completeness of the JSON structure in consumer-misbehaviour.json.
# Test: Check JSON structure against schema definition.
echo "Verify JSON structure against schema definition."
Length of output: 106
Script:
#!/bin/bash
# Description: Verify the consistency and completeness of the JSON structure in consumer-misbehaviour.json.
# Locate the JSON file
json_file=$(fd consumer-misbehaviour.json)
# Check if the file exists
if [[ -z "$json_file" ]]; then
echo "JSON file not found."
exit 1
fi
# Validate JSON structure using jq
jq empty "$json_file" 2>/dev/null
if [[ $? -ne 0 ]]; then
echo "Invalid JSON structure."
exit 1
fi
# Check for consistency in the JSON structure
jq -e '.[0] as $first | .[] | keys == $first | all' "$json_file" >/dev/null
if [[ $? -ne 0 ]]; then
echo "Inconsistent JSON structure."
exit 1
fi
echo "JSON structure is valid and consistent."
Length of output: 579
.changelog/unreleased/api-breaking/1964-deprecate-soft-opt-out.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
docs/docs/adrs/adr-009-soft-opt-out.md (2)
Line range hint
25-25: Grammar correction needed in the context section.Consider using either the past participle “run” or the present participle “running” here.
- a function is ran + a function is runTools
LanguageTool
[uncategorized] ~17-~17: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...needed to validate all consumer chains. Therefore a need exists to allow the bottomx%...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Markdownlint
15-15: Expected: 1; Actual: 0; Above
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
Line range hint
25-25: Add a determiner for clarity.A determiner may be missing before "smallest non opt-out voting power".
- the so called _smallest non opt-out voting power_ + the so-called _smallest non opt-out voting power_Tools
LanguageTool
[uncategorized] ~17-~17: A comma may be missing after the conjunctive/linking adverb ‘Therefore’.
Context: ...needed to validate all consumer chains. Therefore a need exists to allow the bottomx%...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Markdownlint
15-15: Expected: 1; Actual: 0; Above
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
tests/e2e/json_marshal_test.go
Outdated
| @@ -15,7 +15,7 @@ import ( | |||
|
|
|||
| func TestProposalUnmarshal(t *testing.T) { | |||
| proposalAndTypeString := `{ | |||
| "Type": "main.ConsumerAdditionProposal", | |||
| "Type": "e2e.ConsumerAdditionProposal", | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Mismatch in JSON string and struct fields.
The JSON string proposalAndTypeString does not contain the expected Type and Status fields with values "e2e.ConsumerAdditionProposal" and 3, respectively. Please ensure these fields are correctly specified in the JSON string.
- Line 18: JSON string assignment should include
"Type": "e2e.ConsumerAdditionProposal"and"Status": 3.
Analysis chain
Ensure JSON consistency with struct.
The JSON string and the expectedProposal struct must be consistent. Verify that the Type and Status fields match the expected values.
Ensure JSON consistency with struct.
The JSON string and the expectedProposal struct must be consistent. Verify that the Type and Status fields match the expected values.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the JSON consistency with the struct.
# Test: Ensure the JSON string and the struct have matching fields and values.
rg --type go $'proposalAndTypeString' | xargs -I {} grep -q 'Type.*e2e.ConsumerAdditionProposal' {} && echo "Type field matches" || echo "Type field mismatch"
rg --type go $'proposalAndTypeString' | xargs -I {} grep -q 'Status.*3' {} && echo "Status field matches" || echo "Status field mismatch"
Length of output: 730
Script:
#!/bin/bash
# Description: Verify the JSON consistency with the struct.
# Locate the JSON string assignment and ensure it contains the expected fields and values.
rg --pcre2 'proposalAndTypeString\s*:=\s*`.*"Type":\s*"e2e\.ConsumerAdditionProposal".*`' tests/e2e/json_marshal_test.go && echo "Type field matches" || echo "Type field mismatch"
rg --pcre2 'proposalAndTypeString\s*:=\s*`.*"Status":\s*3.*`' tests/e2e/json_marshal_test.go && echo "Status field matches" || echo "Status field mismatch"
Length of output: 319
tests/e2e/json_marshal_test.go
Outdated
| @@ -120,7 +120,7 @@ | |||
| }, | |||
| "Proposals": { | |||
| "1": { | |||
| "Type": "main.NotAProposalTypeProposal", | |||
| "Type": "e2e.NotAProposalTypeProposal", | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Mismatch in JSON Type field and expected error message.
The JSON Type field does not match the expected value e2e.NotAProposalTypeProposal, and the expectedUnmarshalErrorText containing "not a known proposal type" is present but not aligned with the JSON string.
jsonByteswithType: "e2e.NotAProposalTypeProposal"is missing.expectedUnmarshalErrorTextcontaining "not a known proposal type" is present but not matching the JSONType.
Please review and correct the test cases to ensure consistency.
Analysis chain
Ensure JSON consistency with expected error message.
The JSON string and the expectedUnmarshalErrorText must be consistent. Verify that the Type field matches the expected unknown proposal type and the error message is correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the JSON consistency with the expected error message.
# Test: Ensure the JSON string and the expected error message have matching fields and values.
rg --type go $'jsonBytes' | xargs -I {} grep -q 'Type.*e2e.NotAProposalTypeProposal' {} && echo "Type field matches" || echo "Type field mismatch"
rg --type go $'expectedUnmarshalErrorText' | xargs -I {} grep -q 'not a known proposal type' {} && echo "Error message matches" || echo "Error message mismatch"
Length of output: 2610
Script:
#!/bin/bash
# Description: Locate the exact content and context of jsonBytes and expectedUnmarshalErrorText variables in the tests/e2e/json_marshal_test.go file.
# Search for jsonBytes variable definition and usage
rg --type go 'jsonBytes' tests/e2e/json_marshal_test.go
# Search for expectedUnmarshalErrorText variable definition and usage
rg --type go 'expectedUnmarshalErrorText' tests/e2e/json_marshal_test.go
Length of output: 1115
Script:
#!/bin/bash
# Description: Extract the specific test cases that include the jsonBytes with Type: "e2e.NotAProposalTypeProposal" and the expectedUnmarshalErrorText with "not a known proposal type".
# Extract test cases with jsonBytes containing Type: "e2e.NotAProposalTypeProposal"
rg --type go -A 5 'jsonBytes:.*Type.*e2e.NotAProposalTypeProposal' tests/e2e/json_marshal_test.go
# Extract test cases with expectedUnmarshalErrorText containing "not a known proposal type"
rg --type go -A 5 'expectedUnmarshalErrorText:.*not a known proposal type' tests/e2e/json_marshal_test.go
Length of output: 344
insumity
force-pushed
the
insumity/backport-1964
branch
from
371cf97 to
5c13905
Compare
.changelog/unreleased/api-breaking/1995-deprecate-soft-opt-out.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (5)
tests/e2e/tracehandler_testdata/shorthappy.json (1)
64-67: New fieldsValidatorsPowerCap,ValidatorSetCap,Allowlist, andDenylistare not found in the schema definitions.The introduction of these fields should be validated against the schema updates to ensure consistency and correctness.
- Ensure that the schema files are updated to include definitions for
ValidatorsPowerCap,ValidatorSetCap,Allowlist, andDenylist.Analysis chain
Introduced new fields under
TopNsection.The introduction of
ValidatorsPowerCap,ValidatorSetCap,Allowlist, andDenylistshould be validated against the schema updates and intended test scenarios.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of the new fields in the JSON schema. # Test: Search for the new fields in the JSON schema. Expect: Consistent definition. rg --type json -A 5 $'ValidatorsPowerCap|ValidatorSetCap|Allowlist|Denylist'Length of output: 6867
Script:
#!/bin/bash # Description: Verify the consistency of the new fields in the JSON schema files. # Test: Search for the new fields in the JSON schema files. Expect: Consistent definition. rg --type json -A 5 $'ValidatorsPowerCap|ValidatorSetCap|Allowlist|Denylist' | grep -i 'schema'Length of output: 94
Script:
#!/bin/bash # Description: Find JSON schema files and verify the consistency of the new fields. # Step 1: Find all JSON schema files. schema_files=$(fd --type f --extension json schema) # Step 2: Search for the new fields in the found schema files. for file in $schema_files; do echo "Checking file: $file" rg --type json -A 5 $'ValidatorsPowerCap|ValidatorSetCap|Allowlist|Denylist' "$file" doneLength of output: 54
Script:
#!/bin/bash # Description: Search for common schema keywords in all JSON files to locate schema definitions. # Step 1: Find all JSON files. json_files=$(fd --type f --extension json) # Step 2: Search for common schema keywords in the found JSON files. for file in $json_files; do echo "Checking file: $file" rg --type json -A 5 $'properties|definitions' "$file" doneLength of output: 10580
Script:
#!/bin/bash # Description: Search for new fields within the context of schema definitions in all JSON files. # Step 1: Find all JSON files. json_files=$(fd --type f --extension json) # Step 2: Search for the new fields within the context of schema definitions in the found JSON files. for file in $json_files; do echo "Checking file: $file" rg --type json -A 10 -B 10 $'ValidatorsPowerCap|ValidatorSetCap|Allowlist|Denylist' "$file" doneLength of output: 12094
tests/e2e/tracehandler_testdata/happyPath.json (1)
101-101: Review of Updated"Type"ValuesThe update from
"main.ConsumerAdditionProposal"to"e2e.ConsumerAdditionProposal"and"main.ConsumerRemovalProposal"to"e2e.ConsumerRemovalProposal"reflects a change in the namespace or context, likely for testing purposes. This should be clearly documented to avoid confusion about the usage of these types in different environments.Consider adding comments in the JSON or associated documentation to explain the context of these
e2etypes.Also applies to: 256-256, 1492-1492, 1542-1542, 1585-1585, 1633-1633
tests/e2e/steps_partial_set_security.go (2)
1490-1493: Confirm addition of trailing comma inAllowlist.The addition of a trailing comma in the
Allowlistarray is syntactically correct but may be unnecessary unless this array is intended to be frequently updated. If this is not the case, removing the trailing comma could slightly clean up the code.
2212-2215: Confirm addition of trailing comma inAllowlist.The trailing comma in the
Allowlistarray is added, which is syntactically correct. However, if there are no frequent future additions expected to this list, it might be cleaner to remove the comma.x/ccv/consumer/types/genesis_test.go (1)
Unused imports found in
x/ccv/consumer/types/genesis_test.go.The following imported packages are not used in the file and should be removed to avoid unnecessary dependencies:
sdkPlease review and clean up the imports accordingly.
Analysis chain
Line range hint
3-12: Review imported packages for relevance and correctness.The imports include several packages from
ibc-go,cosmos-sdk, and local modules. Ensure that all these imports are used in the file, which helps avoid unnecessary dependencies.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unused imports in the file. ast-grep --lang go --pattern 'import $$$' x/ccv/consumer/types/genesis_test.goLength of output: 1518
Script:
#!/bin/bash # Description: Check for usage of each imported package in the file. # List of imported packages packages=( "testing" "time" "clienttypes" "commitmenttypes" "ibctmtypes" "require" "sdk" "stakingtypes" "abci" "tmtypes" "crypto" "types" "ccv" ) # Search for usage of each package in the file for pkg in "${packages[@]}"; do echo "Searching for usage of package: $pkg" rg "$pkg" x/ccv/consumer/types/genesis_test.go doneLength of output: 19533
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (7)
tests/e2e/tracehandler_testdata/slashThrottle.json (1)
101-101: Update All References to Proposal TypesThe change from
main.ConsumerAdditionProposaltoe2e.ConsumerAdditionProposalhas not been fully propagated throughout the codebase. The following files still contain references to the old proposal types and need to be updated:
tests/e2e/testlib/types.go
- Line containing:
case "main.ConsumerAdditionProposal":tests/e2e/json_marshal_test.go
- Lines containing:
"Type": "main.ConsumerAdditionProposal"Please ensure these references are updated to maintain consistency and avoid potential issues.
Analysis chain
Change in Proposal Types from
maintoe2eThe change in proposal types from
main.ConsumerAdditionProposaltoe2e.ConsumerAdditionProposaland frommain.ConsumerRemovalProposaltoe2e.ConsumerRemovalProposalis noted. This change likely aims to better organize or differentiate the testing environments or functionalities.Please ensure that:
- All references in the codebase are updated to reflect this change.
- Related documentation is updated.
- No functionality is inadvertently affected by this change.
Also applies to: 256-256
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to the old proposal types are updated. # Test: Search for old proposal types. Expect: No occurrences. rg --type json 'main.ConsumerAdditionProposal' || rg --type json 'main.ConsumerRemovalProposal'Length of output: 92
Script:
#!/bin/bash # Description: Verify that all references to the old proposal types are updated across the entire codebase. # Test: Search for old proposal types in all file types. Expect: No occurrences. rg 'main.ConsumerAdditionProposal' || rg 'main.ConsumerRemovalProposal'Length of output: 251
tests/e2e/tracehandler_testdata/democracy.json (2)
101-101: Update all references to "main.ConsumerAdditionProposal"There are still occurrences of the old type "main.ConsumerAdditionProposal" in the codebase. Please update these references to "e2e.ConsumerAdditionProposal" to ensure consistency.
tests/e2e/testlib/types.gotests/e2e/json_marshal_test.goAnalysis chain
Type change from "main.ConsumerAdditionProposal" to "e2e.ConsumerAdditionProposal"
This change reflects a shift from a main namespace to an end-to-end testing namespace, likely for better modularity and testing isolation. Confirm that all references to the old type have been updated across the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the update of "ConsumerAdditionProposal" type across the project. # Test: Search for old type usage. Expect: No occurrences of the old type. rg --type json -A 5 $'main.ConsumerAdditionProposal'Length of output: 51
Script:
#!/bin/bash # Description: Verify the update of "ConsumerAdditionProposal" type across the project. # Test: Search for old type usage across the entire codebase. Expect: No occurrences of the old type. rg 'main.ConsumerAdditionProposal' -A 5 # Test: Search for new type usage across the entire codebase. Expect: Occurrences of the new type. rg 'e2e.ConsumerAdditionProposal' -A 5Length of output: 10220
644-644: Type change from "main.IBCTransferParamsProposal" to "e2e.IBCTransferParamsProposal"The type change for
IBCTransferParamsProposalfrom themainto thee2enamespace is partially implemented. The old typemain.IBCTransferParamsProposalis still present in the following file and should be updated to ensure consistency:
tests/e2e/testlib/types.go:case "main.IBCTransferParamsProposal":Please update this instance to
e2e.IBCTransferParamsProposal.Analysis chain
Type change from "main.IBCTransferParamsProposal" to "e2e.IBCTransferParamsProposal"
The type change for IBCTransferParamsProposal from the main to the e2e namespace is likely for consistency with testing practices. Ensure that this change is reflected everywhere in the project.
Also applies to: 694-694
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the update of "IBCTransferParamsProposal" type across the project. # Test: Search for old type usage. Expect: No occurrences of the old type. rg --type json -A 5 $'main.IBCTransferParamsProposal'Length of output: 52
Script:
#!/bin/bash # Description: Verify the update of "IBCTransferParamsProposal" type across the project. # Test: Search for old type usage in all file types. Expect: No occurrences of the old type. rg 'main.IBCTransferParamsProposal' # Test: Search for new type usage in all file types. Expect: Occurrences of the new type where relevant. rg 'e2e.IBCTransferParamsProposal'Length of output: 719
tests/e2e/tracehandler_testdata/shorthappy.json (2)
101-101: Update references to "ConsumerAdditionProposal" typeThe type "main.ConsumerAdditionProposal" still exists in the following files and needs to be updated to "e2e.ConsumerAdditionProposal":
tests/e2e/json_marshal_test.gotests/e2e/testlib/types.goPlease update these references to ensure consistency across the project.
Analysis chain
Type change from "main.ConsumerAdditionProposal" to "e2e.ConsumerAdditionProposal"
This type change is consistent with the previous file's changes, reflecting a shift to an end-to-end testing namespace. Confirm that all references to the old type have been updated across the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the update of "ConsumerAdditionProposal" type across the project. # Test: Search for old type usage. Expect: No occurrences of the old type. rg --type json -A 5 $'main.ConsumerAdditionProposal'Length of output: 51
Script:
#!/bin/bash # Description: Verify the update of "ConsumerAdditionProposal" type across the project. # Test: Search for old type usage across all file types. Expect: No occurrences of the old type. rg 'main.ConsumerAdditionProposal' -A 5Length of output: 1052
1202-1202: References to the old type "main.ConsumerRemovalProposal" still existThe type "main.ConsumerRemovalProposal" has not been fully updated to "e2e.ConsumerRemovalProposal" across the project. Please update the following occurrences:
tests/e2e/testlib/types.go: The old type is still being used.Analysis chain
Type change from "main.ConsumerRemovalProposal" to "e2e.ConsumerRemovalProposal"
The type change for ConsumerRemovalProposal from the main to the e2e namespace is consistent with the changes made for AdditionProposal. Verify that all references to the old type have been updated across the project.
Also applies to: 1252-1252, 1295-1295
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the update of "ConsumerRemovalProposal" type across the project. # Test: Search for old type usage. Expect: No occurrences of the old type. rg --type json -A 5 $'main.ConsumerRemovalProposal'Length of output: 50
Script:
#!/bin/bash # Description: Verify the update of "ConsumerRemovalProposal" type across the project. # Test: Search for old type usage in all files. Expect: No occurrences of the old type. rg -A 5 'main.ConsumerRemovalProposal' # Test: Search for new type usage in all files. Expect: Occurrences of the new type. rg -A 5 'e2e.ConsumerRemovalProposal'Length of output: 5110
tests/e2e/steps_partial_set_security.go (2)
Line range hint
14-14: Reminder: Add tests.The TODO comment indicates that tests are missing.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Line range hint
24-29:
Optimize the modulo operation.The initial if condition is redundant and the while loop can be replaced with a modulo operation.
- if x < y: - return x - while x >= y: - x -= y - return x + return x % y
|
Hey, can we remove this? it's unused interchain-security/x/ccv/types/params.go Line 31 in 43883e3 |
| SmallestNonOptOutPowerByteKey | ||
| // NOTE: This key is deprecated, but left in place to avoid consumer state migrations | ||
| // [DEPRECATED] | ||
| DeprecatedSmallestNonOptOutPowerByteKey |
There was a problem hiding this comment.
I like the rename, we didn't do this for other deprecated keys; not sure what the implications are, but I think it's ok to have it like this.
There was a problem hiding this comment.
The renaming should not have an impact on anything. We just leave DeprecatedSmallestNonOptOutPowerByteKey there, so the byte value of it (i.e., 10) is not reused by some other key/prefix, because otherwise, this would break the store.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
x/ccv/types/params.go (1)
Line range hint
127-163: Validation logic inValidatefunction looks good.The comprehensive checks for each parameter ensure robust validation. The code adheres to the Uber Golang style guide.
Consider adding specific tests to ensure that the deprecated
SoftOptOutThresholdbehaves as expected when set to "0".Would you like me to help create these tests or should I open a GitHub issue to track this task?
Description
Backporting #1964
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...
!to the type prefix if the change is state-machine breakingCHANGELOG.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.
I have...
!the type prefix if the change is state-machine breakingSummary by CodeRabbit
Deprecations
Improvements
Bug Fixes
Documentation