go-wrapper for Stellar #8
Conversation
nguyen-zung
requested review from
seunghwalee-cramiumlabs,
zkrypt-crossbar and
sookyung-eom
WalkthroughThis change adds support for Internet Computer Protocol (ICP) and Stellar coins to the Go wrapper. It introduces new coin type constants, sample transaction test functions for both coins, utility constants for Stellar passphrases, and a wallet information printing function. The main function is updated to invoke the new sample tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant Sample
participant Core
participant Protobuf
Main->>Sample: TestICP()
Sample->>Core: Create ICP wallet from mnemonic
Sample->>Core: Build ICP SigningInput (protobuf)
Sample->>Protobuf: Marshal SigningInput
Sample->>Core: PreImageHashes (get hash for signing)
Core-->>Sample: PreSigningOutput (protobuf)
Sample->>Protobuf: Unmarshal PreSigningOutput
Sample->>Main: Print data hash
sequenceDiagram
participant Main
participant Sample
participant Core
participant Protobuf
Main->>Sample: TestStellar()
Sample->>Core: Build Stellar SigningInput (protobuf)
Sample->>Protobuf: Marshal SigningInput
Sample->>Core: PreImageHashes (get hash for signing)
Core-->>Sample: PreSigningOutput (protobuf)
Sample->>Protobuf: Unmarshal PreSigningOutput
Sample->>Core: PublicKeyVerify (verify signature)
Sample->>Core: CompileWithSignatures (assemble signed tx)
Core-->>Sample: SigningOutput (protobuf)
Sample->>Main: Print signed transaction
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (3)
wrapper/go-wrapper/sample/icp.go (1)
18-18: Fix incorrect comment.The comment says "bitcoin wallet" but the code creates an ICP wallet.
- // bitcoin wallet + // ICP walletwrapper/go-wrapper/sample/stellar.go (1)
33-38: Improve error handling consistency.The error handling is inconsistent -
proto.Marshalerror is ignored whileproto.Unmarshalerror is also ignored. Consider handling marshaling errors for robustness.- txInputData, _ := proto.Marshal(input) + txInputData, err := proto.Marshal(input) + if err != nil { + panic(fmt.Errorf("failed to marshal input: %w", err)) + }- proto.Unmarshal(preimage, &preSigningOutput) + err = proto.Unmarshal(preimage, &preSigningOutput) + if err != nil { + panic(fmt.Errorf("failed to unmarshal preimage: %w", err)) + }wrapper/go-wrapper/sample/near.go (1)
40-46: Improve error handling consistency.Similar to the stellar.go file, error handling is inconsistent. The marshaling error is ignored while unmarshaling error is also ignored.
- txInputData, _ := proto.Marshal(&input) + txInputData, err := proto.Marshal(&input) + if err != nil { + panic(fmt.Errorf("failed to marshal input: %w", err)) + }- proto.Unmarshal(msgForSign, &preSigningOutput) + err = proto.Unmarshal(msgForSign, &preSigningOutput) + if err != nil { + panic(fmt.Errorf("failed to unmarshal preimage: %w", err)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wrapper/go-wrapper/core/coin.go(1 hunks)wrapper/go-wrapper/core/stellar.go(1 hunks)wrapper/go-wrapper/core/utils.go(1 hunks)wrapper/go-wrapper/main.go(1 hunks)wrapper/go-wrapper/sample/icp.go(1 hunks)wrapper/go-wrapper/sample/near.go(1 hunks)wrapper/go-wrapper/sample/stellar.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
wrapper/go-wrapper/core/stellar.go
1-1: : # github.com/Cramiumlabs/wallet-core/wrapper/go-wrapper/core
wrapper/go-wrapper/core/utils.go:5:20: undefined: Wallet
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: test-wasm
- GitHub Check: build
- GitHub Check: build-and-test
- GitHub Check: build
🔇 Additional comments (13)
wrapper/go-wrapper/main.go (1)
58-59: LGTM! Consistent integration of new sample tests.The addition of
sample.TestICP()andsample.TestStellar()follows the established pattern and integrates well with the existing sample test calls.wrapper/go-wrapper/core/coin.go (1)
24-25: LGTM! Proper coin type constant definitions.The new
CoinTypeICPandCoinTypeStellarconstants are correctly defined following the established pattern and use appropriate Trust Wallet Core C constants.wrapper/go-wrapper/core/stellar.go (1)
3-4: LGTM! Well-defined Stellar network constants.The Stellar passphrase constants are properly defined with descriptive names and follow Go naming conventions. These appear to be standard Stellar network identifiers.
wrapper/go-wrapper/sample/icp.go (2)
23-23: Dependent on WalletInfo compilation fix.This line calls
core.WalletInfo()which has a compilation error due to the undefinedWallettype. This will need to be fixed once the issue inwrapper/go-wrapper/core/utils.gois resolved.
25-36: LGTM! Well-structured ICP transaction input.The SigningInput construction for the ICP transfer transaction is well-structured and includes all necessary fields for a proper transfer operation.
wrapper/go-wrapper/sample/stellar.go (4)
1-12: LGTM! Clean package structure and imports.The package declaration and imports are well-organized and appropriate for the functionality.
14-31: LGTM! Well-structured Stellar transaction input.The SigningInput construction covers all essential fields for a Stellar payment transaction including passphrase, account details, operation, and memo. The hardcoded values are appropriate for a test function.
40-47: LGTM! Proper signature verification with appropriate error handling.The signature verification logic is correct with proper error handling using panic for test failure, which is appropriate for a test function.
49-57: Good transaction compilation with proper error handling.The transaction compilation and output handling demonstrate proper error checking and appropriate panic behavior for test scenarios.
wrapper/go-wrapper/sample/near.go (4)
1-12: LGTM! Consistent package structure.The imports and package declaration are appropriate and consistent with the stellar.go implementation.
16-23: LGTM! Appropriate test data setup.The hardcoded test data is well-organized, and the transfer amount initialization demonstrates proper byte array handling for NEAR's 128-bit amounts.
24-40: LGTM! Well-structured NEAR transaction input.The SigningInput construction properly demonstrates a NEAR transfer transaction with all required fields including signer/receiver IDs, nonce, block hash, and transfer action.
48-63: LGTM! Proper signature verification and transaction compilation.The signature verification and transaction compilation logic is correct with appropriate error handling. The output formatting with both signed transaction and hash provides good debugging information.
| func WalletInfo(w *Wallet) { | ||
| fmt.Printf("%s wallet: \n", w.CoinType.GetName()) | ||
| fmt.Printf("\t address: %s \n", w.Address) | ||
| fmt.Printf("\t pri key: %s \n", w.PriKey) | ||
| fmt.Printf("\t pub key: %s \n", w.PubKey) | ||
| } |
There was a problem hiding this comment.
Fix compilation error: undefined Wallet type.
The Wallet type is not defined or imported in this package, causing a compilation error as indicated by the static analysis tool.
The Wallet type needs to be imported or this function should be moved to a package where Wallet is defined. Looking at the codebase, Wallet appears to be defined in the same core package, so you may need to check if there's a missing file or if the Wallet struct definition needs to be added.
Additionally, this function duplicates the printWallet function from main.go. Consider consolidating this functionality.
+// Add the Wallet struct definition or import it from the appropriate location
func WalletInfo(w *Wallet) {
fmt.Printf("%s wallet: \n", w.CoinType.GetName())
fmt.Printf("\t address: %s \n", w.Address)
fmt.Printf("\t pri key: %s \n", w.PriKey)
fmt.Printf("\t pub key: %s \n", w.PubKey)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wrapper/go-wrapper/core/utils.go around lines 5 to 10, the Wallet type is
undefined causing a compilation error. Verify if the Wallet struct is defined
elsewhere in the core package or if its definition is missing and add it here if
needed. Alternatively, move this WalletInfo function to the package where Wallet
is defined. Also, consider removing this duplicate function and consolidating
with the existing printWallet function in main.go to avoid redundancy.
| txInputData, _ := proto.Marshal(input) | ||
| msgForSign := core.PreImageHashes(core.CoinTypeICP, txInputData) | ||
|
|
||
| fmt.Println("msgForSign:", msgForSign) | ||
|
|
||
| var preSigningOutput transactioncompiler.PreSigningOutput | ||
| proto.Unmarshal(msgForSign, &preSigningOutput) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for protobuf operations.
The proto.Marshal and proto.Unmarshal operations can fail but errors are currently ignored, which could lead to runtime panics or unexpected behavior.
- txInputData, _ := proto.Marshal(input)
+ txInputData, err := proto.Marshal(input)
+ if err != nil {
+ panic(err)
+ }
msgForSign := core.PreImageHashes(core.CoinTypeICP, txInputData)
fmt.Println("msgForSign:", msgForSign)
var preSigningOutput transactioncompiler.PreSigningOutput
- proto.Unmarshal(msgForSign, &preSigningOutput)
+ err = proto.Unmarshal(msgForSign, &preSigningOutput)
+ if err != nil {
+ panic(err)
+ }📝 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.
| txInputData, _ := proto.Marshal(input) | |
| msgForSign := core.PreImageHashes(core.CoinTypeICP, txInputData) | |
| fmt.Println("msgForSign:", msgForSign) | |
| var preSigningOutput transactioncompiler.PreSigningOutput | |
| proto.Unmarshal(msgForSign, &preSigningOutput) | |
| txInputData, err := proto.Marshal(input) | |
| if err != nil { | |
| panic(err) | |
| } | |
| msgForSign := core.PreImageHashes(core.CoinTypeICP, txInputData) | |
| fmt.Println("msgForSign:", msgForSign) | |
| var preSigningOutput transactioncompiler.PreSigningOutput | |
| err = proto.Unmarshal(msgForSign, &preSigningOutput) | |
| if err != nil { | |
| panic(err) | |
| } |
🤖 Prompt for AI Agents
In wrapper/go-wrapper/sample/icp.go around lines 39 to 45, the proto.Marshal and
proto.Unmarshal calls ignore errors, risking runtime panics or unexpected
behavior. Modify the code to capture and check the errors returned by
proto.Marshal and proto.Unmarshal. If an error occurs, handle it appropriately
by returning the error or logging it, ensuring the program does not proceed with
invalid data.
Thank you!
Summary by CodeRabbit
New Features
Tests