feat(gosdk): migrate Go-sdk into the Nibiru blockchain repo. #1893
Conversation
WalkthroughThe recent update ushers in significant enhancements to the Nibiru blockchain repository, including refined Changes
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
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 Configration 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: 9
Outside diff range and nitpick comments (1)
gosdk/gosdk_test.go (1)
92-98: Clarify the purpose of commented-out FIXME.The FIXME comment regarding the node home for a local validator is vague. It would be beneficial to either address this FIXME or provide more details about what needs to be fixed or investigated.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.modis excluded by!**/*.mod,!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (19)
- .gitignore (1 hunks)
- CHANGELOG.md (1 hunks)
- app/server/start.go (1 hunks)
- app/server/util.go (1 hunks)
- gosdk/README.md (1 hunks)
- gosdk/broadcast.go (1 hunks)
- gosdk/clientctx.go (1 hunks)
- gosdk/export_test.go (1 hunks)
- gosdk/gosdk.go (1 hunks)
- gosdk/gosdk_test.go (1 hunks)
- gosdk/grpc.go (1 hunks)
- gosdk/keys.go (1 hunks)
- gosdk/keys_test.go (1 hunks)
- gosdk/netinfo.go (1 hunks)
- gosdk/querier.go (1 hunks)
- gosdk/rpc.go (1 hunks)
- gosdk/sequence_test.go (1 hunks)
- justfile (1 hunks)
- x/common/testutil/cli/network.go (2 hunks)
Files skipped from review due to trivial changes (6)
- .gitignore
- app/server/start.go
- app/server/util.go
- gosdk/netinfo.go
- gosdk/sequence_test.go
- justfile
Additional Context Used
LanguageTool (1)
gosdk/README.md (1)
Near line 51: You might be missing the article “the” here.
Context: ...ld steps, we may benefit from splitting "start" command from the bulk of the subc...
Markdownlint (4)
CHANGELOG.md (1)
63: Expected: 0 or 2; Actual: 1
Trailing spacesgosdk/README.md (3)
3: Expected: 0 or 2; Actual: 1
Trailing spaces
9: Expected: 0 or 2; Actual: 1
Trailing spaces
43: Expected: 0 or 2; Actual: 1
Trailing spaces
Additional comments not posted (5)
gosdk/keys_test.go (2)
13-53: Good use of table-driven tests inTestCreateSigner.
49-53: Properly checks for non-panic behavior inTestKeyring.gosdk/querier.go (1)
17-52: Well-structuredQuerierstruct and robust nil handling inNewQuerier.gosdk/export_test.go (1)
23-84: Comprehensive setup functions inCreateBlockchainand related helpers.CHANGELOG.md (1)
92-92: The changelog entry for PR #1893 is correctly formatted and provides an appropriate summary of the changes introduced by the PR.
| node has more cumbersome dependencies like RocksDB that involve C-Go and compled | ||
| build steps, we may benefit from splitting "start" command from the bulk of the | ||
| subcommands available on teh Nibiru CLI. This would make it much easier to have a | ||
| command line tool that builds on Linux, Windows, Mac. |
There was a problem hiding this comment.
Consider adding the article "the" before "start" command for grammatical correctness.
- we may benefit from splitting "start" command from the bulk of the subcommands available on teh Nibiru CLI.
+ we may benefit from splitting the "start" command from the bulk of the subcommands available on the Nibiru CLI.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.
| node has more cumbersome dependencies like RocksDB that involve C-Go and compled | |
| build steps, we may benefit from splitting "start" command from the bulk of the | |
| subcommands available on teh Nibiru CLI. This would make it much easier to have a | |
| command line tool that builds on Linux, Windows, Mac. | |
| node has more cumbersome dependencies like RocksDB that involve C-Go and compled | |
| build steps, we may benefit from splitting the "start" command from the bulk of the | |
| subcommands available on the Nibiru CLI. This would make it much easier to have a | |
| command line tool that builds on Linux, Windows, Mac. |
| // TODO: Is this needed? | ||
| // import ( | ||
| // "bufio" | ||
| // "os" | ||
| // "path/filepath" | ||
| // ) | ||
| // func NewKeyringLocal(nodeDir string) (keyring.Keyring) { | ||
| // clientDir := filepath.Join(nodeDir, "keyring") | ||
| // var cdc codec.Codec = EncodingConfig.Marshaler | ||
| // buf := bufio.NewReader(os.Stdin) | ||
| // return keyring.New( | ||
| // sdk.KeyringServiceName(), | ||
| // keyring.BackendTest, | ||
| // clientDir, | ||
| // buf, | ||
| // cdc, | ||
| // ) | ||
| // } |
There was a problem hiding this comment.
Consider removing or implementing the commented-out code for NewKeyringLocal.
The commented-out code for NewKeyringLocal function is present but not active. If this function is not needed, it should be removed to keep the codebase clean. If it is potentially useful, consider implementing it or moving it to a feature branch until it is ready.
| func AddSignerToKeyring( | ||
| kring keyring.Keyring, privKey cryptotypes.PrivKey, keyName string, | ||
| ) error { | ||
| passphrase := "password" | ||
| armor := crypto.EncryptArmorPrivKey(privKey, passphrase, privKey.Type()) | ||
| return kring.ImportPrivKey(keyName, armor, passphrase) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that the hardcoded passphrase "password" is indeed used in the AddSignerToKeyring function in gosdk/keys.go. However, there are no indications from the search results that this is a placeholder or that a more secure method of handling passphrases is implemented elsewhere in the codebase.
Points of Concern:
- Hardcoded Passphrase: The use of a hardcoded passphrase "password" in the
AddSignerToKeyringfunction is a potential security risk. - Lack of Secure Handling: There is no evidence from the search results that a more secure method of handling passphrases is implemented elsewhere in the codebase.
Recommendations:
- Replace Hardcoded Passphrase: Consider replacing the hardcoded passphrase with a more secure method, such as prompting the user for a passphrase or retrieving it from a secure environment variable.
- Documentation and Comments: If this hardcoded passphrase is intended as a placeholder, it should be clearly documented with comments indicating that it needs to be replaced in a production environment.
Actionable Steps:
- Review and Refactor: Review the
AddSignerToKeyringfunction and refactor it to handle passphrases securely. - Add Comments: Add comments to the code to indicate that the hardcoded passphrase is a placeholder and should be replaced in production.
Code Location:
gosdk/keys.go: Line 77-83
Analysis chain
Ensure sensitive data handling is secure.
The function AddSignerToKeyring uses a hardcoded passphrase, which might pose a security risk. Verify if this is a placeholder or if a more secure method of handling passphrases is implemented elsewhere in the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of hardcoded passphrase in key management functions.
rg --type python "password"
Length of output: 58
Script:
#!/bin/bash
# Description: Search for usage of hardcoded passphrase in Go files.
rg --type go "password"
Length of output: 32053
| func NewNibiruSdk( | ||
| chainId string, | ||
| grpcConn *grpc.ClientConn, | ||
| rpcEndpt string, | ||
| ) (NibiruSDK, error) { | ||
| EnsureNibiruPrefix() | ||
| encCfg := app.MakeEncodingConfig() | ||
| keyring := keyring.NewInMemory(encCfg.Codec) | ||
| queryClient, err := NewQuerier(grpcConn) | ||
| if err != nil { | ||
| return NibiruSDK{}, err | ||
| } | ||
| cometRpc, err := NewRPCClient(rpcEndpt, "/websocket") | ||
| if err != nil { | ||
| return NibiruSDK{}, err | ||
| } | ||
| return NibiruSDK{ | ||
| ChainId: chainId, | ||
| Keyring: keyring, | ||
| EncCfg: encCfg, | ||
| Querier: queryClient, | ||
| CometRPC: cometRpc, | ||
| AccountRetriever: authtypes.AccountRetriever{}, | ||
| GrpcClient: grpcConn, | ||
| }, err | ||
| } |
There was a problem hiding this comment.
Ensure error handling is robust in NewNibiruSdk.
The function NewNibiruSdk initializes several components and returns the first error encountered. Consider adding cleanup logic or using a defer statement to handle resources properly in case of an error.
- if err != nil {
- return NibiruSDK{}, err
+ if err != nil {
+ // Cleanup or close resources if needed
+ return NibiruSDK{}, errCommittable suggestion was skipped due low confidence.
| func BroadcastMsgsWithSeq( | ||
| args BroadcastArgs, | ||
| from sdk.AccAddress, | ||
| seq uint64, | ||
| msgs ...sdk.Msg, | ||
| ) (*sdk.TxResponse, error) { | ||
| broadcaster := args.Broadcaster | ||
|
|
||
| info, err := args.kring.KeyByAddress(from) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| txBuilder := args.txCfg.NewTxBuilder() | ||
| err = txBuilder.SetMsgs(msgs...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| bondDenom := denoms.NIBI | ||
| txBuilder.SetFeeAmount(sdk.NewCoins(sdk.NewCoin(bondDenom, sdk.NewInt(1000)))) | ||
| txBuilder.SetGasLimit(uint64(2 * common.TO_MICRO)) | ||
|
|
||
| nums, err := args.gosdk.GetAccountNumbers(from.String()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var accRetriever sdkclient.AccountRetriever = authtypes.AccountRetriever{} | ||
| txFactory := sdkclienttx.Factory{}. | ||
| WithChainID(args.chainID). | ||
| WithKeybase(args.kring). | ||
| WithTxConfig(args.txCfg). | ||
| WithAccountRetriever(accRetriever). | ||
| WithAccountNumber(nums.Number). | ||
| WithSequence(seq) | ||
|
|
||
| overwriteSig := true | ||
| err = sdkclienttx.Sign(txFactory, info.Name, txBuilder, overwriteSig) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| txBytes, err := args.txCfg.TxEncoder()(txBuilder.GetTx()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return broadcaster.BroadcastTxSync(txBytes) | ||
| } |
There was a problem hiding this comment.
Review transaction broadcasting logic for potential improvements.
The function BroadcastMsgsWithSeq sets up a transaction and broadcasts it. Consider refactoring to separate the concerns more clearly, such as separating transaction creation from broadcasting.
| func (n *Network) WaitForNextBlockVerbose() (int64, error) { | ||
| lastBlock, err := n.LatestHeight() | ||
| if err != nil { | ||
| return err | ||
| return -1, err | ||
| } | ||
|
|
||
| _, err = n.WaitForHeight(lastBlock + 1) | ||
| newBlock := lastBlock + 1 | ||
| _, err = n.WaitForHeight(newBlock) | ||
| if err != nil { | ||
| return err | ||
| return lastBlock, err | ||
| } | ||
|
|
||
| return newBlock, err | ||
| } |
There was a problem hiding this comment.
Ensure error handling is robust in WaitForNextBlockVerbose.
The function WaitForNextBlockVerbose handles errors but does not provide detailed logging or recovery options. Consider enhancing error handling to provide more context or recovery options.
| // NewRPCClient: A remote Comet-BFT RPC client. An error is returned on | ||
| // invalid remote. The function panics when remote is nil. | ||
| // | ||
| // Args: | ||
| // - rpcEndpt: endpoint in the form <protocol>://<host>:<port> | ||
| // - websocket: websocket path (which always seems to be "/websocket") | ||
| func NewRPCClient(rpcEndpt string, websocket string) (*cmtrpchttp.HTTP, error) { | ||
| return cmtrpchttp.New(rpcEndpt, websocket) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure explicit panic handling for nil rpcEndpt in NewRPCClient or update the documentation to reflect the actual behavior.
gosdk/rpc.go: Add explicit nil checks forrpcEndptor update the function documentation to remove the mention of panic.
Analysis chain
Ensure explicit panic handling for nil rpcEndpt.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if there's any handling for nil `rpcEndpt` in the function `NewRPCClient`.
# Test: Search for nil checks in `NewRPCClient`. Expect: Explicit nil checks or panic handling.
ast-grep --lang go --pattern $'func NewRPCClient($_, $_) {
$$$
if $_ == nil {
panic($_)
}
$$$
}'
Length of output: 109
Script:
#!/bin/bash
# Description: Verify if `cmtrpchttp.New` handles nil `rpcEndpt`.
# Locate the definition of `cmtrpchttp.New`.
ast-grep --lang go --pattern $'func New($_, $_) ($_,$_) {
$$$
}' --search 'cmtrpchttp'
# Locate all usages of `NewRPCClient` to check for nil checks.
rg 'NewRPCClient'
Length of output: 634
Script:
#!/bin/bash
# Description: Verify if `cmtrpchttp.New` handles nil `rpcEndpt`.
# Locate the definition of `cmtrpchttp.New`.
ast-grep --lang go --pattern $'func New($_, $_) ($_,$_) {
$$$
}' cmtrpchttp
# Inspect the usages of `NewRPCClient` in `gosdk/gosdk.go` and `gosdk/clientctx.go` for nil checks.
rg 'NewRPCClient' gosdk/gosdk.go gosdk/clientctx.go
Length of output: 338
Script:
#!/bin/bash
# Description: Locate the `cmtrpchttp` package and inspect `gosdk/gosdk.go` for nil checks.
# Locate the `cmtrpchttp` package.
fd cmtrpchttp
# Inspect the `gosdk/gosdk.go` file for nil checks around `NewRPCClient`.
rg 'NewRPCClient' -A 5 -B 5 gosdk/gosdk.go
Length of output: 341
Script:
#!/bin/bash
# Description: Search for similar package names and inspect import statements in the codebase.
# Search for similar package names.
fd -t d cmtrp
# Inspect import statements in the codebase to locate the `cmtrpchttp` package.
rg 'import' --glob '*.go'
Length of output: 137930
| if grpcInsecure { | ||
| creds = insecure.NewCredentials() | ||
| } else { | ||
| creds = credentials.NewTLS(&tls.Config{}) |
There was a problem hiding this comment.
Set a minimum TLS version to enhance security.
- creds = credentials.NewTLS(&tls.Config{})
+ creds = credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS13})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.
| creds = credentials.NewTLS(&tls.Config{}) | |
| creds = credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS13}) |
gosdk/clientctx.go
Outdated
| // TODO: Make a way to instantiate a NibiruSDK from a | ||
| // (*cli.Network, *cli.Validator) | ||
|
|
||
| // ClientCtx: Docs for args | ||
| // | ||
| // - tmCfgRootDir: /node0/simd | ||
| // - Validator.Dir: /node0 | ||
| // - Validator.ClientCtx.KeyringDir: /node0/simcli | ||
|
|
||
| // func NewNibiruSDKFromClientCtx( | ||
| // clientCtx sdkclient.Context, grpcUrl, cometRpcUrl string, | ||
| // ) (gosdk NibiruSDK, err error) { | ||
| // grpcConn, err := GetGRPCConnection(grpcUrl, true, 5) | ||
| // if err != nil { | ||
| // return | ||
| // } | ||
| // cometRpc, err := NewRPCClient(cometRpcUrl, "/websocket") | ||
| // if err != nil { | ||
| // return | ||
| // } | ||
| // querier, err := NewQuerier(grpcConn) | ||
| // if err != nil { | ||
| // return | ||
| // } | ||
| // return NibiruSDK{ | ||
| // ChainId: clientCtx.ChainID, | ||
| // Keyring: clientCtx.Keyring, | ||
| // EncCfg: app.MakeEncodingConfig(), | ||
| // Querier: querier, | ||
| // CometRPC: cometRpc, | ||
| // AccountRetriever: authtypes.AccountRetriever{}, | ||
| // GrpcClient: grpcConn, | ||
| // }, err | ||
| // } |
There was a problem hiding this comment.
Finalize or remove the commented-out NewNibiruSDKFromClientCtx function.
Would you like me to help finalize this function or should we track this task in a GitHub issue?
There was a problem hiding this comment.
github issue, please
There was a problem hiding this comment.
@Unique-Divine, I have created a GitHub issue to track the task of finalizing the NewNibiruSDKFromClientCtx function. You can find the issue here: Issue #1894.
Feel free to add any additional details or context to the issue as needed.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue in the NibiruChain/nibiru repository
gh issue create --title "Finalize the NewNibiruSDKFromClientCtx function" --body "There is a need to finalize the implementation of the `NewNibiruSDKFromClientCtx` function in the `gosdk/clientctx.go` file. This function is crucial for creating a `NibiruSDK` instance from specified parameters.
Please refer to the pull request for more context: https://github.com/NibiruChain/nibiru/pull/1893
Comment for reference: https://github.com/NibiruChain/nibiru/pull/1893#discussion_r1611877768" --repo NibiruChain/nibiru --assignee Unique-Divine
Length of output: 855
New Learnings Added
User: Unique-Divine
PR: NibiruChain/nibiru#1893
File: gosdk/clientctx.go:9-42
Timestamp: 2024-05-23T15:58:29.054Z
Learning: When a user requests to track a task in a GitHub issue, create the issue using the GitHub CLI with a clear title and body, including links to the relevant PR and comment for context.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1893 +/- ##
==========================================
+ Coverage 67.17% 67.37% +0.20%
==========================================
Files 212 218 +6
Lines 13447 13641 +194
==========================================
+ Hits 9033 9191 +158
- Misses 3768 3790 +22
- Partials 646 660 +14
|
Purpose / Abstract