Migrate to oasis sdk #208
Migrate to oasis sdk #208
Conversation
…encies. migrate: use Oasis SDK for join, clean secp256k1, fix RPC hangs Move join contract reads to the Oasis SDK (Sapphire) and update Go module dependencies accordingly. Drop the vendored libsecp256k1 tree and its submodule metadata, relying on the geth secp256k1 wrapper instead. Add bounded RPC response waiting and ensure client initialization only starts after a successful connection to avoid indefinite startup hangs.
Summary of ChangesHello @tuhalf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request marks a significant architectural shift by integrating the Oasis SDK into the project. The primary goal is to modernize and streamline interactions with the Oasis network, moving away from a custom JSON-RPC implementation and a C-based cryptographic library. This change impacts how smart contract data is fetched and how elliptic curve operations are handled, leading to a more cohesive Go-native codebase. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request primarily integrates the Oasis SDK for client-side interactions, replacing a custom JSON-RPC implementation. This involves adding new Oasis SDK imports, removing old HTTP-related imports, and refactoring the joinHandler and getPropertyValues functions in cmd/diode/join.go to use the new OasisClient struct and its methods. The rpcURL variable has been removed, and contract address handling now uses contractAddrBytes and oasisClient. Concurrently, the crypto/secp256k1 submodule has been removed, and its related Cgo bindings and ScalarMult function in curve.go have been eliminated, with corresponding adjustments to ext.h and the Add method in curve.go to handle points at infinity. A review comment suggests making OASIS_LOCAL_SAPPHIRE_ID a required environment variable for local network setups to improve explicitness, and another points out the magic number 20 for contract address length, recommending a named constant for better readability.
| } | ||
| sapphireID := strings.TrimSpace(os.Getenv(oasisLocalSapphireIDEnv)) | ||
| if sapphireID == "" { | ||
| sapphireID = defaultSapphireID() |
There was a problem hiding this comment.
The defaultSapphireID function falls back to using the 'testnet' configuration to get a default Sapphire ID for the 'local' network. This dependency on testnet configuration for a local setup might be confusing for developers and makes the local environment not self-contained.
Consider making OASIS_LOCAL_SAPPHIRE_ID a required environment variable when network=local. This would make the configuration more explicit and less surprising.
| return fmt.Errorf("invalid network: %s", network) | ||
| var err error | ||
| contractAddrBytes, err = hex.DecodeString(strings.TrimPrefix(contractAddress, "0x")) | ||
| if err != nil || len(contractAddrBytes) != 20 { |
There was a problem hiding this comment.
The number 20 is used here to check the length of the contract address. This is a magic number. To improve readability and maintainability, consider defining a constant for this value, for example const expectedAddressLength = 20, and using that constant here.
|
I sometimes get this runtime error: goroutine 52 [running]: |
No description provided.