Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix inactive vals and min stake handling in ConsumerAdditionProposal conversion #2181

Merged
merged 7 commits into from
Aug 28, 2024
Merged

fix: Fix inactive vals and min stake handling in ConsumerAdditionProposal conversion #2181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c16992c
Add inactive vals and min stake to Chain struct
p-offtermatt Aug 28, 2024
d712823
Merge branch 'main' of https://github.com/cosmos/interchain-security
p-offtermatt Aug 28, 2024
062ee57
Add allow_inactive_vals and min_stake to consumer chain query result
p-offtermatt Aug 28, 2024
8aedb09
Add inactive vals and min stake to consumer chain getter test
p-offtermatt Aug 28, 2024
975d583
Fix regression
p-offtermatt Aug 28, 2024
ec73f3e
Revert extra logging changes
p-offtermatt Aug 28, 2024
6367057
Remove e2e scenario
p-offtermatt Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions proto/interchain_security/ccv/provider/v1/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,11 @@ message Chain {
repeated string allowlist = 7;
// Corresponds to a list of provider consensus addresses of validators that CANNOT validate the consumer chain.
repeated string denylist = 8;
// Corresponds to the minimal amount of (provider chain) stake required to validate on the consumer chain.
uint64 min_stake = 9;
// Corresponds to whether inactive validators are allowed to validate the consumer chain.
bool allow_inactive_vals = 10;

}

message QueryValidatorConsumerAddrRequest {
Expand Down
15 changes: 11 additions & 4 deletions tests/e2e/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,9 @@ func (tr Chain) submitConsumerAdditionProposal(
"validator_set_cap": %d,
"allowlist": %s,
"denylist": %s,
"authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn"
"authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Issue: Exposed API Key

The hardcoded API key in the JSON template might expose sensitive operations to unauthorized access.

Consider removing the hardcoded API key or replacing it with a secure method of fetching it, such as environment variables or secure vault solutions.

-   "authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn",
+   "authority": os.Getenv("AUTHORITY_KEY"),

Ensure to import the os package at the beginning of the file.

Committable suggestion was skipped due to low confidence.

Tools
Gitleaks

302-302: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

"allow_inactive_vals": %t,
"min_stake": "%d"
}
],
"metadata": "ipfs://CID",
Expand Down Expand Up @@ -327,6 +329,8 @@ func (tr Chain) submitConsumerAdditionProposal(
action.ValidatorSetCap,
action.Allowlist,
action.Denylist,
action.AllowInactiveVals,
action.MinStake,
action.Deposit)

//#nosec G204 -- bypass unsafe quoting warning (no production code)
Expand Down Expand Up @@ -587,13 +591,14 @@ type SubmitConsumerModificationProposalAction struct {
ValidatorSetCap uint32
Allowlist []string
Denylist []string
AllowInactiveVals bool
MinStake uint64
Comment on lines +594 to +595
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor Suggestion: Consistent Parameter Ordering

The addition of AllowInactiveVals and MinStake parameters in SubmitConsumerModificationProposalAction struct should follow a consistent order across similar functions to improve readability and maintainability.

Consider reordering the parameters to match the order used in other related structs or function signatures for consistency.

 type SubmitConsumerModificationProposalAction struct {
 	Chain              ChainID
 	From               ValidatorID
 	Deposit            uint
 	ConsumerChain      ChainID
 	TopN               uint32
 	ValidatorsPowerCap uint32
 	ValidatorSetCap    uint32
 	Allowlist          []string
 	Denylist           []string
+	AllowInactiveVals  bool
+	MinStake           uint64
-	AllowInactiveVals  bool
-	MinStake           uint64
 }

Committable suggestion was skipped due to low confidence.

}

func (tr Chain) submitConsumerModificationProposal(
action SubmitConsumerModificationProposalAction,
verbose bool,
) {

template := `

{
Expand All @@ -609,8 +614,8 @@ func (tr Chain) submitConsumerModificationProposal(
"allowlist": %s,
"denylist": %s,
"authority": "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn",
"min_stake": "0",
"allow_inactive_vals": false
"min_stake": %d,
"allow_inactive_vals": %t
Comment on lines +617 to +618
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Quality Improvement: Use Struct for Parameters

The function submitConsumerModificationProposal uses many parameters directly in the JSON template string. This can be error-prone and hard to maintain.

Consider defining a struct for these parameters and marshal it to JSON. This approach enhances code readability and maintainability.

type ConsumerModificationParams struct {
    MinStake           uint64 `json:"min_stake"`
    AllowInactiveVals  bool   `json:"allow_inactive_vals"`
    // include other fields...
}

params := ConsumerModificationParams{
    MinStake:          action.MinStake,
    AllowInactiveVals: action.AllowInactiveVals,
    // set other fields...
}
jsonStr, err := json.Marshal(params)
if err != nil {
    log.Fatal(err)
}

Use this jsonStr in your template.

}
],
"metadata": "ipfs://CID",
Expand All @@ -629,6 +634,8 @@ func (tr Chain) submitConsumerModificationProposal(
action.Allowlist,
action.Denylist,
action.Deposit,
action.MinStake,
action.AllowInactiveVals,
)

// #nosec G204 -- bypass unsafe quoting warning (no production code)
Expand Down
6 changes: 6 additions & 0 deletions x/ccv/provider/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ func (k Keeper) GetConsumerChain(ctx sdk.Context, chainID string) (types.Chain,
strDenylist[i] = addr.String()
}

allowInactiveVals := k.AllowsInactiveValidators(ctx, chainID)

minStake, _ := k.GetMinStake(ctx, chainID)

return types.Chain{
ChainId: chainID,
ClientId: clientID,
Expand All @@ -104,6 +108,8 @@ func (k Keeper) GetConsumerChain(ctx sdk.Context, chainID string) (types.Chain,
ValidatorsPowerCap: validatorsPowerCap,
Allowlist: strAllowlist,
Denylist: strDenylist,
AllowInactiveVals: allowInactiveVals,
MinStake: minStake,
}, nil
}

Expand Down
13 changes: 13 additions & 0 deletions x/ccv/provider/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,15 @@ func TestGetConsumerChain(t *testing.T) {
{},
}

allowInactiveVals := []bool{true, false, true, false}

minStakes := []math.Int{
math.NewInt(0),
math.NewInt(100),
math.NewInt(200),
math.NewInt(300),
}

expectedGetAllOrder := []types.Chain{}
for i, chainID := range chainIDs {
clientID := fmt.Sprintf("client-%d", len(chainIDs)-i)
Expand All @@ -289,6 +298,8 @@ func TestGetConsumerChain(t *testing.T) {
pk.SetValidatorSetCap(ctx, chainID, validatorSetCaps[i])
pk.SetValidatorsPowerCap(ctx, chainID, validatorPowerCaps[i])
pk.SetMinimumPowerInTopN(ctx, chainID, expectedMinPowerInTopNs[i])
pk.SetInactiveValidatorsAllowed(ctx, chainID, allowInactiveVals[i])
pk.SetMinStake(ctx, chainID, minStakes[i].Uint64())
for _, addr := range allowlists[i] {
pk.SetAllowlist(ctx, chainID, addr)
}
Expand All @@ -315,6 +326,8 @@ func TestGetConsumerChain(t *testing.T) {
ValidatorsPowerCap: validatorPowerCaps[i],
Allowlist: strAllowlist,
Denylist: strDenylist,
AllowInactiveVals: allowInactiveVals[i],
MinStake: minStakes[i].Uint64(),
})
}

Expand Down
2 changes: 2 additions & 0 deletions x/ccv/provider/keeper/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ func (h Hooks) GetConsumerAdditionFromProp(
ValidatorSetCap: sdkMsg.ValidatorSetCap,
Allowlist: sdkMsg.Allowlist,
Denylist: sdkMsg.Denylist,
MinStake: sdkMsg.MinStake,
AllowInactiveVals: sdkMsg.AllowInactiveVals,
}
return proposal, true
}
Expand Down
2 changes: 2 additions & 0 deletions x/ccv/provider/keeper/proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ func (k Keeper) HandleConsumerAdditionProposal(ctx sdk.Context, proposal *types.
ValidatorSetCap: proposal.ValidatorSetCap,
Allowlist: proposal.Allowlist,
Denylist: proposal.Denylist,
MinStake: proposal.MinStake,
AllowInactiveVals: proposal.AllowInactiveVals,
}

return k.HandleLegacyConsumerAdditionProposal(ctx, &p)
Expand Down
Loading
Loading