Make iaviewer usage more human friendly #719
Conversation
WalkthroughThe changes introduce a significant enhancement to the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/scripts/iaviewer/iaviewer.go (4 hunks)
- protocol/scripts/iaviewer/unmarshaller.go (1 hunks)
Additional comments: 12
protocol/scripts/iaviewer/unmarshaller.go (2)
1-9: The imports look fine. Ensure that all the imported packages are being used in the code.
21-28: The
unmarshallerRegistryis a map of maps, storing message names and corresponding unmarshaller functions. This is a good approach for organizing the unmarshallers. However, ensure that the keys used in the map are consistent and accurate across the codebase.protocol/scripts/iaviewer/iaviewer.go (10)
9-16: The import section has been updated to include the
cobralibrary for command-line parsing and theregexppackage for regular expressions. Theosandstrconvpackages are no longer imported, as they are not used in the new code.27-41: The
mainfunction has been refactored to use thecobralibrary for command-line parsing. TherootCmdvariable is defined as acobra.Commandand is executed in themainfunction. Theinitfunction adds four new commands torootCmd:DataAllCmd,DataCmd,ShapeCmd, andVersionsCmd.43-56: Three new functions have been added to add flags to a
cobra.Command:AddPathFlag,AddPrefixFlag, andAddVersionFlag. These functions add a "path", "prefix", and "version" flag to the command, respectively.57-87: The
DataAllCmdfunction has been added. This function returns acobra.Commandthat retrieves data from all IAVL trees in the LevelDB directory specified by the "path" flag. If the "version" flag is provided, it retrieves data from the specified version of the trees; otherwise, it retrieves data from the latest version.89-118: The
DataCmdfunction has been added. This function returns acobra.Commandthat retrieves data from a specific IAVL tree in the LevelDB directory specified by the "path" flag and the "prefix" flag. If the "version" flag is provided, it retrieves data from the specified version of the tree; otherwise, it retrieves data from the latest version.120-146: The
ShapeCmdfunction has been added. This function returns acobra.Commandthat retrieves the shape of a specific IAVL tree in the LevelDB directory specified by the "path" flag and the "prefix" flag. If the "version" flag is provided, it retrieves the shape of the specified version of the tree; otherwise, it retrieves the shape of the latest version.148-173: The
VersionsCmdfunction has been added. This function returns acobra.Commandthat lists the versions of a specific IAVL tree in the LevelDB directory specified by the "path" flag and the "prefix" flag. If the "version" flag is provided, it retrieves the versions of the specified version of the tree; otherwise, it retrieves the versions of the latest version.204-227: The
GetAllPrefixesfunction has been added. This function fetches all prefixes for IAVL trees in the DB by iterating through all keys and looking for prefixes that match what is expected from modules.249-258: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [253-267]
The
ReadTreefunction has been updated to accept adbm.DBand auint64for the version instead of a directory string and an integer. The function now prints the version of the tree being read.
- 269-292: The
PrintTreefunction has been updated to print the data of a specific IAVL tree. It now accepts a*iavl.MutableTreeand a prefix string as arguments. The function prints the keys, hashed values, and human-readable strings of the protobuf messages if they are in the registry. It also prints the hash and size of the tree.
| func protoUnmarshaller[M codec.ProtoMarshaler](b []byte) string { | ||
| cdc := app.GetEncodingConfig().Codec | ||
| var m M | ||
| // TODO: avoid reflection? | ||
| msgType := reflect.TypeOf(m).Elem() | ||
| m = reflect.New(msgType).Interface().(M) | ||
| cdc.MustUnmarshal(b, m) | ||
| return m.String() | ||
| } |
There was a problem hiding this comment.
The protoUnmarshaller function uses reflection to create a new instance of the type M and then unmarshals the byte slice into it. While this is a valid approach, reflection can be slow and error-prone. If the type M does not implement the codec.ProtoMarshaler interface, this function will panic at runtime. Consider adding error handling to make the function more robust.
11:
func protoUnmarshaller[M codec.ProtoMarshaler](b []byte) (string, error) {
12:
cdc := app.GetEncodingConfig().Codec
13:
var m M
14:
// TODO: avoid reflection?
15:
msgType := reflect.TypeOf(m).Elem()
16:
m = reflect.New(msgType).Interface().(M)
17:
err := cdc.Unmarshal(b, m)
18:
if err != nil {
19:
return "", err
20:
}
21:
return m.String(), nil
22:
}Commitable suggestion (Beta)
| func protoUnmarshaller[M codec.ProtoMarshaler](b []byte) string { | |
| cdc := app.GetEncodingConfig().Codec | |
| var m M | |
| // TODO: avoid reflection? | |
| msgType := reflect.TypeOf(m).Elem() | |
| m = reflect.New(msgType).Interface().(M) | |
| cdc.MustUnmarshal(b, m) | |
| return m.String() | |
| } | |
| func protoUnmarshaller[M codec.ProtoMarshaler](b []byte) (string, error) { | |
| cdc := app.GetEncodingConfig().Codec | |
| var m M | |
| // TODO: avoid reflection? | |
| msgType := reflect.TypeOf(m).Elem() | |
| m = reflect.New(msgType).Interface().(M) | |
| err := cdc.Unmarshal(b, m) | |
| if err != nil { | |
| return "", err | |
| } | |
| return m.String(), nil | |
| } |
There was a problem hiding this comment.
I want this to panic for now.
There was a problem hiding this comment.
@roy-dydx Understood. If you want the function to panic in case of an error during unmarshalling, you can continue using MustUnmarshal. This will indeed cause a panic if the unmarshalling fails. Please feel free to reach out if you need further assistance or have any other requests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/scripts/iaviewer/unmarshaller.go (1 hunks)
Additional comments: 2
protocol/scripts/iaviewer/unmarshaller.go (2)
11-19: The previous comment about adding error handling to the
protoUnmarshallerfunction is still valid. The function currently usesMustUnmarshalwhich will panic if unmarshalling fails. It would be better to useUnmarshaland return an error if it fails. This will make the function more robust and prevent unexpected panics.21-32: The
unmarshallerRegistrymap is a good way to associate prefixes with unmarshalling functions. However, it's important to ensure that the prefixes used are unique and do not overlap. If they do, it could lead to unexpected behavior. Consider adding checks to ensure that the prefixes are unique and do not overlap.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/scripts/iaviewer/iaviewer.go (4 hunks)
Additional comments: 7
protocol/scripts/iaviewer/iaviewer.go (7)
28-31: The
rootCmdis defined correctly and theSilenceUsageflag is set totrueto prevent the usage message from being printed when an error occurs. This is a good practice for a cleaner CLI experience.34-38: The error handling in the
mainfunction is done correctly. If an error occurs during the execution of therootCmd, it is printed toos.Stderrand the program exits with a non-zero status code.40-45: The
initfunction is used to add commands to therootCmd. This is a good practice as it ensures that the commands are added before themainfunction is executed.61-63: The
AddPathFlag,AddPrefixFlag, andAddVersionFlagfunctions are used to add flags to the commands. This is a good practice as it makes the code more modular and easier to maintain.156-182: The
DataAllCmd,DataCmd,ShapeCmd, andVersionsCmdfunctions are used to create the commands. These functions are well-structured and make good use of thecobralibrary's features. The error handling in theRunEfunctions is done correctly.260-266: The
ReadTreefunction is used to load an iavl tree from a DB. If a prefix is provided, a newPrefixDBis created with the provided prefix. This is a good practice as it allows for more granular control over the data that is loaded from the DB.277-300: The
PrintTreefunction is used to print the data of an iavl tree. The function iterates through the tree and prints each key and its corresponding value. The function also checks if there is a registered unmarshaller for the tree's prefix and uses it to unmarshal the value if there is one. This is a good practice as it allows for more detailed output.
| // GetAllPrefixes fetches all prefixes for iavl trees in the DB. There's no great way to do this, | ||
| // so we just iterate through all keys and look for prefixes that match what we expect from modules. | ||
| func GetAllPrefixes(db dbm.DB) []string { | ||
| it, err := db.Iterator(nil, nil) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| defer it.Close() | ||
| // Luckily for us, this iterator goes in alphabetical order | ||
| var prefixes []string | ||
| for ; it.Valid(); it.Next() { | ||
| k := it.Key() | ||
| matches := prefixRe.FindStringSubmatch(string(k)) | ||
| if matches != nil { | ||
| if len(prefixes) == 0 || matches[1] != prefixes[len(prefixes)-1] { | ||
| prefixes = append(prefixes, matches[1]) | ||
| } | ||
| } | ||
| } | ||
| if err := it.Error(); err != nil { | ||
| panic(err) | ||
| } | ||
| return prefixes |
There was a problem hiding this comment.
The GetAllPrefixes function is used to fetch all prefixes for iavl trees in the DB. The function iterates through all keys in the DB and looks for prefixes that match what is expected from modules. This function could potentially be optimized by using a more efficient method to fetch the prefixes, but without more context, it's hard to suggest a specific improvement.
| msgType := reflect.TypeOf(m).Elem() | ||
| m = reflect.New(msgType).Interface().(M) | ||
| cdc.MustUnmarshal(b, m) | ||
| return m.String() |
There was a problem hiding this comment.
Q: should we return string here and use %s? or should we return m and use %+v?
not sure if %+v works
There was a problem hiding this comment.
The functions would have generic return value which results in different return values for each proto. We need all these functions to be the same signature so we can put them in a map like we're doing now.
There was a problem hiding this comment.
you can use any?
not a big deal though
There was a problem hiding this comment.
Then to use String() I'd need to type assert which would require another generic function. (or maybe reflection works?)
I don't know how %+v works, but I don't think it would work on interface{}.
| cmd.Flags().Uint64("version", 0, "version of the tree (default: latest version)") | ||
| } | ||
|
|
||
| func DataAllCmd() *cobra.Command { |
There was a problem hiding this comment.
for my understanding - how did this work before prefix option was added? kind of surprised that this is not already supported
There was a problem hiding this comment.
It doesn't work. If you don't pass prefix in, it will assume that all the leveldb keys are unprefixed and try to interpret it as an iavl tree. But for dbs created by cosmos, all the keys are prefixed and not ready to be interpreted as iavl tree unless the prefix is removed. So you'd just find no values unless you pass in a prefix of a tree.
Changelist
data-allcommand to list iavl tree for all modulesTest Plan
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.