Conversation
…etwork When agent restarts with docker-compose, a new network is created. Existing containers (created before restart) may not be connected to this new network. This fix checks and reconnects containers to the correct network when they are reused.
Shorten data URI prefix from 'data:application/json;base64,' (30 bytes) to 'data:;base64,' (13 bytes), saving 17 bytes per on-chain transaction. This optimization reduces L1 calldata costs on Arbitrum L2. The short format is valid per RFC 2397 - MIME type defaults to text/plain.
- Update CommitmentUtils test: remove redundancy, add verifierFee - Update contracts-mock test: add coordinator and verifierFee fields - Update PayloadUtils test: expect short data URI format (data:;base64,) - Update storage test: expect short data URI format
- Update contract ABIs to remove redundancy field from Commitment - Add verifierFee field to Commitment type - Update Coordinator and Router wrappers for new contract interface - Sync typechain generated types with updated ABIs - Update agent-core to handle new Commitment structure - Remove redundancy from ComputeSubscription type - Update tests for new contract structure All 497 tests passing.
|
@gemini-code-assist review |
Summary of ChangesHello @nolnol3, 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 focuses on refining the Noosphere SDK's core functionalities by optimizing smart contract interactions for reduced gas consumption and simplifying the underlying data models. It introduces a more efficient way to handle transaction costs and clarifies the commitment tracking mechanism by removing redundant parameters. Additionally, it improves the reliability of local development environments through better Docker container network handling. Highlights
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
This pull request introduces a significant and well-executed refactoring to remove the "redundancy" feature and optimize gas usage. The changes are consistently applied across contracts, ABIs, core logic, and tests. Key improvements include replacing the redundancy mechanism with a simpler commitment check, adding support for EIP-2930 access lists for gas optimization, and enhancing the agent's robustness with better error handling and logging. My review focuses on further improving code robustness and maintainability by replacing hardcoded values and adding safer type checks. Overall, this is a high-quality update that modernizes the SDK's architecture.
| const provider = this.contract.runner?.provider as ethers.JsonRpcProvider | undefined; | ||
| console.log(` 📋 createAccessList: provider=${!!provider}, send=${typeof provider?.send}`); | ||
| if (!provider || typeof provider.send !== 'function') { | ||
| console.warn(' ⚠️ No JsonRpcProvider available for access list creation'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Casting the provider to JsonRpcProvider directly can be risky. If the runner is configured with a different provider type (e.g., FallbackProvider), this could lead to a runtime error when trying to access provider.send. The eth_createAccessList RPC method is specific to JSON-RPC providers and might not be available on other provider types.
A safer approach is to perform an instanceof check to ensure the provider is indeed a JsonRpcProvider before using its specific methods.
| const provider = this.contract.runner?.provider as ethers.JsonRpcProvider | undefined; | |
| console.log(` 📋 createAccessList: provider=${!!provider}, send=${typeof provider?.send}`); | |
| if (!provider || typeof provider.send !== 'function') { | |
| console.warn(' ⚠️ No JsonRpcProvider available for access list creation'); | |
| return null; | |
| } | |
| const provider = this.contract.runner?.provider; | |
| console.log(` 📋 createAccessList: provider=${!!provider}`); | |
| if (!(provider instanceof ethers.JsonRpcProvider)) { | |
| console.warn(' ⚠️ No JsonRpcProvider available for access list creation'); | |
| return null; | |
| } |
| console.log(` ⏭️ Already fulfilled (${currentCount}/${event.redundancy}), skipping`); | ||
| // Check if already fulfilled (commitment deleted check) | ||
| const commitmentHash = await this.coordinator.requestCommitments(event.requestId); | ||
| const isAlreadyFulfilled = commitmentHash === '0x0000000000000000000000000000000000000000000000000000000000000000'; |
There was a problem hiding this comment.
For better readability and maintainability, it's recommended to use the ethers.ZeroHash constant instead of a hardcoded zero hash string. This makes the code cleaner and less prone to typos.
| const isAlreadyFulfilled = commitmentHash === '0x0000000000000000000000000000000000000000000000000000000000000000'; | |
| const isAlreadyFulfilled = commitmentHash === ethers.ZeroHash; |
| ); | ||
| return redundancy > 0; | ||
| // If commitment hash is non-zero, commitment exists | ||
| return commitmentHash !== '0x0000000000000000000000000000000000000000000000000000000000000000'; |
There was a problem hiding this comment.
To improve code clarity and avoid magic strings, it's best to use the ethers.ZeroHash constant for this comparison instead of the hardcoded zero hash.
| return commitmentHash !== '0x0000000000000000000000000000000000000000000000000000000000000000'; | |
| return commitmentHash !== ethers.ZeroHash; |
- Remove package-lock.json to avoid mixed lock file warnings - Regenerate yarn.lock to fix incorrect @types/node@^22.0.0 entry - Fixes CI install failure on Node.js 20.x
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that successfully removes the concept of "redundancy" and introduces gas optimizations. The changes are consistently applied across smart contract ABIs, data structures, agent logic, and tests. Key improvements include a more gas-efficient fulfillment check, an option to use access lists for transactions, and better logging. The code quality is high, with added robustness checks and tooling improvements for the monorepo. I've identified a few areas for minor improvements related to type safety, logging practices, and error message clarity. Overall, this is a solid and well-executed update.
| ]; | ||
|
|
||
| // Build transaction overrides | ||
| const overrides: any = {}; |
There was a problem hiding this comment.
For better type safety, it's recommended to avoid using any. You can define a more specific type for the overrides object to ensure that only valid properties are added.
| const overrides: any = {}; | |
| const overrides: { gasLimit?: bigint; accessList?: AccessList } = {}; |
| if (options?.accessList) { | ||
| // Use provided access list | ||
| overrides.accessList = options.accessList; | ||
| console.log(' 📋 Using provided access list'); |
There was a problem hiding this comment.
Using console.log directly in a library can be problematic as it doesn't allow the consumer of the library to control the verbosity or destination of logs. Consider introducing a lightweight, configurable logger that can be used throughout the SDK. This would provide more flexibility for developers using this package in different environments. This comment applies to other console.log and console.warn statements in this file as well.
| if (!receipt) { | ||
| throw new Error('Transaction receipt is null - transaction may have been dropped'); | ||
| } |
There was a problem hiding this comment.
This is a great check to handle dropped transactions. To make the error even more informative, you could consider making the error message more specific by including the transaction hash. This can be very helpful for debugging network-related issues.
| if (!receipt) { | |
| throw new Error('Transaction receipt is null - transaction may have been dropped'); | |
| } | |
| if (!receipt) { | |
| throw new Error(`Transaction with hash ${tx.hash} was dropped and did not produce a receipt.`); | |
| } |
- Use instanceof check for JsonRpcProvider in Coordinator.ts - Replace hardcoded zero hash with ethers.ZeroHash in: - NoosphereAgent.ts - SchedulerService.ts - PayloadResolver.ts Improves type safety and code maintainability.
1 participant
V2 remove redundancy & optimize gas usage