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

chore: add basic types for legacy params #23543

Closed
wants to merge 2 commits into from
+45 −0
Closed

chore: add basic types for legacy params #23543

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0aad66
legacy params
aljo242 Jan 28, 2025
a2630c1
Merge branch 'main' into chore/legacy-params
aljo242 Jan 29, 2025
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
45 changes: 45 additions & 0 deletions types/legacy/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Package legacy contains types and interfaces that have support removed, but may need to be exported for legacy
Copy link
Member

@julienrbrt julienrbrt Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While x/params is indeed removed from main, its tag remains and is still importable. When do you foresee the need for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ties the types to the actual sdk import as opposed to some other package. At some point there will be a drift between the version of SDK used in an old tag and what a user is using

Copy link
Member

Choose a reason for hiding this comment

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

Are you going to update x/params to use those then? Otherwise it will be moot.

Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/cosmos/cosmos-sdk/pull/23543/files#r1934276529

// and migration purposes.
package legacy

import (
sdk "github.com/cosmos/cosmos-sdk/types"
)

// The following types are from the removed x/params modules and can be used to satisfy
// legacy interfaces that may use these.

type (
ValueValidatorFn func(value interface{}) error

// ParamSetPair is used for associating paramsubspace key and field of param
// structs.
ParamSetPair struct {
Key []byte
Value interface{}
ValidatorFn ValueValidatorFn
}
)

// NewParamSetPair creates a new ParamSetPair instance.
func NewParamSetPair(key []byte, value interface{}, vfn ValueValidatorFn) ParamSetPair {
return ParamSetPair{key, value, vfn}
}
Comment on lines +24 to +27
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

Add input validation to constructor.

The NewParamSetPair function should validate its inputs:

  • Ensure key is not nil/empty
  • Validate value is not nil
  • Verify vfn is not nil

Here's a suggested implementation:

 func NewParamSetPair(key []byte, value interface{}, vfn ValueValidatorFn) ParamSetPair {
+    if len(key) == 0 {
+        panic("parameter key cannot be empty")
+    }
+    if value == nil {
+        panic("parameter value cannot be nil")
+    }
+    if vfn == nil {
+        panic("validator function cannot be nil")
+    }
     return ParamSetPair{key, value, vfn}
 }
📝 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.

Suggested change
// NewParamSetPair creates a new ParamSetPair instance.
func NewParamSetPair(key []byte, value interface{}, vfn ValueValidatorFn) ParamSetPair {
return ParamSetPair{key, value, vfn}
}
// NewParamSetPair creates a new ParamSetPair instance.
func NewParamSetPair(key []byte, value interface{}, vfn ValueValidatorFn) ParamSetPair {
if len(key) == 0 {
panic("parameter key cannot be empty")
}
if value == nil {
panic("parameter value cannot be nil")
}
if vfn == nil {
panic("validator function cannot be nil")
}
return ParamSetPair{key, value, vfn}
}


// ParamSetPairs Slice of KeyFieldPair
type ParamSetPairs []ParamSetPair

// ParamSet defines an interface for structs containing parameters for a module
type ParamSet interface {
ParamSetPairs() ParamSetPairs
Copy link
Member

Choose a reason for hiding this comment

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

because of the type redefinition of this interface, you won't be able to just swap for this. x/params would need to be updated to use this, or it needs to be using structural typing instead.

}

type (
// Subspace defines an interface that implements the legacy x/params Subspace
// type.
//
// NOTE: This is used solely for migration of x/params managed parameters.
Subspace interface {
GetParamSet(ctx sdk.Context, ps ParamSet)
}
)
Loading