update simulation #470
update simulation #470
Conversation
WalkthroughThe pull request encompasses modifications across multiple modules, focusing on simulation and keeper logic. Changes include adjusting genesis state generation in the coinswap and token modules, refining request context handling in the service module's keeper and simulation operations, and introducing more nuanced logic for repeated contexts and parameter initialization. The modifications aim to enhance randomization, validation, and simulation capabilities across different components. Changes
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
modules/service/simulation/operations.go (3)
16-16: Fix import ordering using gci.Please run gci with the following flags to maintain consistent import ordering:
gci --skip-generated -s standard -s default -s prefix(mods.irisnet.org) --custom-order🧰 Tools
🪛 golangci-lint (1.62.2)
16-16: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(mods.irisnet.org) --custom-order(gci)
993-998: Consider making repeated parameters configurable.The current implementation uses fixed values (100 for frequency and 10 for total) when
repeatedis true. Consider making these values configurable or randomized for better simulation coverage.repeated := r.Intn(2) == 0 var repeatedFrequency, repeatedTotal uint64 if repeated { - repeatedFrequency = uint64(100) - repeatedTotal = uint64(10) + repeatedFrequency = uint64(simtypes.RandIntBetween(r, 50, 150)) + repeatedTotal = uint64(simtypes.RandIntBetween(r, 5, 15)) }
1499-1499: Fix comment formatting.Add a space after
//for consistent comment formatting.-//if requestContext.Repeated { +// if requestContext.Repeated {🧰 Tools
🪛 golangci-lint (1.62.2)
1499-1499: commentFormatting: put a space between
//and comment text(gocritic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/coinswap/simulation/genesis.go(1 hunks)modules/service/keeper/invocation.go(1 hunks)modules/service/simulation/operations.go(4 hunks)modules/token/simulation/genesis.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
modules/service/simulation/operations.go
16-16: File is not gci-ed with --skip-generated -s standard -s default -s prefix(mods.irisnet.org) --custom-order
(gci)
1499-1499: commentFormatting: put a space between // and comment text
(gocritic)
🔇 Additional comments (5)
modules/coinswap/simulation/genesis.go (1)
42-42: LGTM: Non-zero pool creation fee ensures valid simulation state.
The change ensures that poolCreationFee is always at least 1 by using r.Int63n(100)+1 instead of just r.Int63n(100). This is a good defensive programming practice that prevents potential issues with zero fees in simulations.
✅ Verification successful
Let me generate additional verification scripts to check the usage of pool creation fee in the actual code logic, not just in type definitions.
Let me try one more verification with a different approach to check the simulation and validation logic.
LGTM: Non-zero pool creation fee is validated and properly handled
The change to ensure non-zero pool creation fee is well-justified based on the codebase evidence:
- The
Params.Validate()method inmodules/coinswap/types/params.goexplicitly requires positive pool creation fee:
if !p.PoolCreationFee.IsPositive() {
return fmt.Errorf("poolCreationFee must be positive: %s", p.PoolCreationFee.String())
}- The fee is actively used in business logic:
DeductPoolCreationFeein both coinswap and farm modules uses it for fee collection- It's used for tax calculations and burning mechanisms
- The simulation operations check for sufficient balance against this fee
The simulation change to use r.Int63n(100)+1 aligns with these validation requirements and prevents potential simulation failures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that pool creation fee is used consistently across the codebase
rg -A 3 "pool.*creation.*fee" --type go
Length of output: 10876
Script:
#!/bin/bash
# Check for pool creation fee validation or usage in business logic
ast-grep --pattern 'func $$$($$$) {
$$$
pool.*creation.*fee
$$$
}'
# Also check for any direct references to pool creation fee in test files
rg "pool.*creation.*fee.*" --type go --glob "*_test.go" -A 3
Length of output: 137
Script:
#!/bin/bash
# Check for pool creation fee validation in simulation and params
rg "poolCreationFee" --type go -B 2 -A 2
# Also check for any validation functions that might handle the fee
ast-grep --pattern 'func $$$Validate($$$) error {
$$$
}'
Length of output: 9812
modules/token/simulation/genesis.go (1)
74-74: Verify GetNativeToken() safety in simulation context.
The change from sdk.DefaultBondDenom to v1.GetNativeToken().Symbol makes the simulation more accurate by using the actual native token symbol. However, we should ensure that GetNativeToken() is safe to call in all simulation contexts.
✅ Verification successful
Let me check the initialization of the Initialized and nativeToken variables to ensure thread safety.
Let me check the initialization of these variables using ripgrep instead.
Let me try a different approach to check the variable declarations.
Based on the code review, I can now provide the final response:
GetNativeToken() is safe to use in simulation context
The implementation shows that GetNativeToken() has proper initialization handling:
- If not initialized, it sets default values including
sdk.DefaultBondDenomas the symbol - The initialization is guarded by a boolean flag
- The function always returns a valid Token struct with sensible defaults
- The default values are hardcoded and do not depend on external state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of GetNativeToken
ast-grep --pattern 'func GetNativeToken() $_ {
$$$
}'
Length of output: 2284
Script:
#!/bin/bash
# Check the declaration of Initialized and nativeToken variables
ast-grep --pattern 'var $_ $_'
Length of output: 65646
Script:
#!/bin/bash
# Check the initialization of Initialized and nativeToken variables
rg -A 2 "var (Initialized|nativeToken)" modules/token/types/v1/genesis.go
Length of output: 75
Script:
#!/bin/bash
# Check the content of the genesis.go file
cat modules/token/types/v1/genesis.go
Length of output: 1765
modules/service/keeper/invocation.go (1)
239-247: LGTM: Improved validation logic for repeated requests.
The change correctly adds a conditional check to validate repeatedFreq only when requestContext.Repeated is true. This fixes a potential issue where the validation was being applied incorrectly to non-repeated requests. The error message is clear and the validation logic ensures that repeated frequency is not less than the timeout, which is essential for proper request scheduling.
modules/service/simulation/operations.go (2)
1164-1172: LGTM! Proper validation for repeated request contexts.
The added check ensures that only repeated request contexts can be paused, which is the correct business logic.
1499-1502: Verify the impact of commented repeated parameters.
The commented code previously initialized repeatedFrequency and repeatedTotal based on the request context's state. Verify if setting these to 0 by default is intentional and won't cause issues.
✅ Verification successful
The previous search showed that these parameters are used in request contexts, but we need to specifically check the validation rules and handling when they are set to 0 to determine if this is intentional and safe.
Based on the validation code found, I can now provide a definitive response about the commented repeated parameters:
Setting repeated parameters to 0 is intentional and safe
The validation code in modules/service/types/validation.go and handling in modules/service/keeper/invocation.go show that:
-
When
repeatedflag is false:repeatedFrequencyis explicitly set to 0repeatedTotalis explicitly set to 0
-
When
repeatedflag is true:repeatedFrequencydefaults to timeout value if 0repeatedTotalmust be either greater than 0 or equal to -1 (validated)
The commented code is safely removed since the default 0 values align with the non-repeated request context behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with repeated parameters being 0
# Search for references to repeatedFrequency and repeatedTotal in request context handling
rg -A 5 'repeatedFrequency|repeatedTotal' --type go
# Search for validation of these parameters
ast-grep --pattern 'if $_ <= 0 {
$$$
}'
Length of output: 65828
Script:
#!/bin/bash
# Search for validation and handling of repeated parameters
rg -A 5 'repeatedFrequency|repeatedTotal' --type go modules/service/types/validation.go modules/service/keeper/invocation.go
# Search for any tests related to repeated parameters
rg -A 5 'repeatedFrequency.*0|repeatedTotal.*0' --type go modules/service/simulation/operations_test.go modules/service/keeper/invocation_test.go
Length of output: 6764
🧰 Tools
🪛 golangci-lint (1.62.2)
1499-1499: commentFormatting: put a space between // and comment text
(gocritic)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
modules/service/simulation/operations.go (2)
16-16: Sort imports to comply with project's import conventionsThe file is not formatted according to the
gciimport sorting tool. Please sort the imports to adhere to the project's conventions.🧰 Tools
🪛 golangci-lint (1.62.2)
16-16: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(mods.irisnet.org) --custom-order(gci)
1165-1172: Improve error message for clarityThe error message
"requestContext non repeated"could be clearer if phrased as"request context is not repeated".Apply this diff to improve the error message:
if !requestContext.Repeated { return simtypes.NoOpMsg( types.ModuleName, types.TypeMsgPauseRequestContext, - "requestContext non repeated", + "request context is not repeated", ), nil, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/service/keeper/invocation.go(1 hunks)modules/service/simulation/operations.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
modules/service/simulation/operations.go
16-16: File is not gci-ed with --skip-generated -s standard -s default -s prefix(mods.irisnet.org) --custom-order
(gci)
🔇 Additional comments (2)
modules/service/keeper/invocation.go (1)
Line range hint 239-244: Ensure consistent use of requestContext.Timeout in condition and error message
The condition compares repeatedFreq with uint64(timeout), while the error message refers to requestContext.Timeout. To avoid inconsistencies, consider using requestContext.Timeout in both the condition and the error message.
Apply this diff to fix the inconsistency:
- if requestContext.Repeated && repeatedFreq < uint64(timeout) {
+ if requestContext.Repeated && repeatedFreq < uint64(requestContext.Timeout) {
return errorsmod.Wrapf(
types.ErrInvalidRepeatedFreq,
"repeated frequency [%d] must not be less than the timeout [%d]",
repeatedFreq,
- requestContext.Timeout,
+ requestContext.Timeout,
)
}modules/service/simulation/operations.go (1)
993-998:
Ensure repeatedFrequency is not less than timeout to prevent validation errors
Currently, repeatedFrequency is set to 100, but timeout can be up to k.MaxRequestTimeout(ctx). If timeout exceeds 100, this will cause a validation error because repeatedFrequency must not be less than timeout. To avoid this issue, consider setting repeatedFrequency to be greater than or equal to timeout when repeated is true.
Apply this diff to fix the issue:
if repeated {
- repeatedFrequency = uint64(100)
+ repeatedFrequency = uint64(timeout)
repeatedTotal = uint64(10)
}Likely invalid or redundant comment.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor