-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add evm provider #6
Conversation
WalkthroughThe pull request introduces support for Ethereum transactions in the Daydreams core library. A new dependency on the Changes
Sequence DiagramsequenceDiagram
participant User
participant ExecuteEvmTransaction
participant EthersProvider
participant EthersWallet
participant SmartContract
User->>ExecuteEvmTransaction: Submit EvmTransaction
ExecuteEvmTransaction->>EthersProvider: Initialize JSON-RPC Provider
ExecuteEvmTransaction->>EthersWallet: Create Wallet Instance
ExecuteEvmTransaction->>SmartContract: Create Contract Instance
ExecuteEvmTransaction->>SmartContract: Execute Contract Method
SmartContract-->>ExecuteEvmTransaction: Return Transaction Receipt
ExecuteEvmTransaction-->>User: Return Transaction Result
Poem
Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (1)
readme.md (1)
3-3
: LGTM! Documentation update reflects new capabilities.The broader scope in the description accurately reflects the addition of EVM support alongside other chains.
Minor suggestion: Consider adding a hyphen to make it "on-chain" for better readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Did you mean “are” or “were”?
Context: # Daydreams Daydreams is a generative agent library for playing ...(SENT_START_NNS_IS)
[uncategorized] ~3-~3: This expression is ususally spelled with a hyphen
Context: ...ary for playing anything onchain. It is chain agnostic and can be used to play anything onchai...(SPECIFIC_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/core/package.json
(1 hunks)packages/core/src/core/providers.ts
(2 hunks)readme.md
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
readme.md
[grammar] ~3-~3: Did you mean “are” or “were”?
Context: # Daydreams Daydreams is a generative agent library for playing ...
(SENT_START_NNS_IS)
[uncategorized] ~3-~3: This expression is ususally spelled with a hyphen
Context: ...ary for playing anything onchain. It is chain agnostic and can be used to play anything onchai...
(SPECIFIC_HYPHEN)
🔇 Additional comments (2)
packages/core/src/core/providers.ts (1)
87-92
: LGTM! Well-structured interface definition.The
EvmTransaction
interface properly extendsCoTTransaction
and includes essential EVM-specific fields.packages/core/package.json (1)
21-21
: Verify ethers.js version compatibility and security.The ethers.js v6.13.5 is a recent version, but let's verify it's free from known vulnerabilities.
Run this script to check for security advisories:
✅ Verification successful
✓ ethers.js v6.13.5 dependency verified
The dependency is secure, up-to-date, and correctly implemented using v6 API patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories in ethers package gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "ethers") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check latest version npm view ethers versions --json | jq -r '.[-1]'Length of output: 395
Script:
#!/bin/bash # Check for peer dependencies and package usage npm view [email protected] peerDependencies --json # Search for ethers imports/requires in the codebase rg -l "require\(['\"]ethers['\"]\)|from ['\"](ethers|@ethersproject)" --type js --type ts # Look for existing ethers version in other package.json files fd package.json --type f --exec grep -l "ethers" {}Length of output: 265
Script:
#!/bin/bash # Check ethers usage in providers.ts cat packages/core/src/core/providers.tsLength of output: 3211
// Initialize provider (customize based on your needs) | ||
const provider = new ethers.JsonRpcProvider(process.env.EVM_RPC_URL); | ||
|
||
// Initialize wallet/signer | ||
const wallet = new ethers.Wallet(process.env.PRIVATE_KEY!, provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add environment variable validation.
The code assumes environment variables will be present without validation.
Add validation before using environment variables:
- const provider = new ethers.JsonRpcProvider(process.env.EVM_RPC_URL);
- const wallet = new ethers.Wallet(process.env.PRIVATE_KEY!, provider);
+ const rpcUrl = process.env.EVM_RPC_URL;
+ const privateKey = process.env.PRIVATE_KEY;
+
+ if (!rpcUrl || !privateKey) {
+ throw new Error('Missing required environment variables: EVM_RPC_URL or PRIVATE_KEY');
+ }
+
+ const provider = new ethers.JsonRpcProvider(rpcUrl);
+ const wallet = new ethers.Wallet(privateKey, provider);
📝 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.
// Initialize provider (customize based on your needs) | |
const provider = new ethers.JsonRpcProvider(process.env.EVM_RPC_URL); | |
// Initialize wallet/signer | |
const wallet = new ethers.Wallet(process.env.PRIVATE_KEY!, provider); | |
// Initialize provider (customize based on your needs) | |
const rpcUrl = process.env.EVM_RPC_URL; | |
const privateKey = process.env.PRIVATE_KEY; | |
if (!rpcUrl || !privateKey) { | |
throw new Error('Missing required environment variables: EVM_RPC_URL or PRIVATE_KEY'); | |
} | |
const provider = new ethers.JsonRpcProvider(rpcUrl); | |
const wallet = new ethers.Wallet(privateKey, provider); |
const txOptions = { | ||
value: BigInt(transaction.value || "0"), | ||
gasLimit: BigInt(transaction.gasLimit || "0"), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve gas limit handling.
The current implementation might lead to transaction failures if gas limit is not properly set.
Consider these improvements:
- Add gas estimation
- Validate gas limit values
- Handle potential out-of-gas scenarios
- const txOptions = {
- value: BigInt(transaction.value || "0"),
- gasLimit: BigInt(transaction.gasLimit || "0"),
- };
+ const txOptions: ethers.ContractTransactionRequest = {
+ value: BigInt(transaction.value || "0"),
+ };
+
+ if (transaction.gasLimit) {
+ txOptions.gasLimit = BigInt(transaction.gasLimit);
+ } else {
+ // Estimate gas if not provided
+ try {
+ txOptions.gasLimit = await contract[transaction.method].estimateGas(
+ ...transaction.calldata,
+ { value: txOptions.value }
+ );
+ } catch (error) {
+ throw new Error(`Gas estimation failed: ${error instanceof Error ? error.message : 'Unknown error'}`);
+ }
+ }
Committable suggestion skipped: line range outside the PR's diff.
const tx = await contract[transaction.method]( | ||
...transaction.calldata, | ||
txOptions | ||
); | ||
const receipt = await tx.wait(); | ||
|
||
return receipt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance transaction execution and receipt handling.
The current implementation lacks proper transaction monitoring and receipt validation.
Consider these improvements:
- Add transaction confirmation blocks
- Validate transaction receipt
- Handle revert reasons
- const tx = await contract[transaction.method](
- ...transaction.calldata,
- txOptions
- );
- const receipt = await tx.wait();
-
- return receipt;
+ try {
+ const tx = await contract[transaction.method](
+ ...transaction.calldata,
+ txOptions
+ );
+
+ // Wait for 2 confirmations
+ const receipt = await tx.wait(2);
+
+ if (!receipt.status) {
+ throw new Error('Transaction failed');
+ }
+
+ return receipt;
+ } catch (error: any) {
+ // Handle revert reasons
+ if (error.data) {
+ throw new Error(`Transaction reverted: ${error.data}`);
+ }
+ throw error;
+ }
📝 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.
const tx = await contract[transaction.method]( | |
...transaction.calldata, | |
txOptions | |
); | |
const receipt = await tx.wait(); | |
return receipt; | |
try { | |
const tx = await contract[transaction.method]( | |
...transaction.calldata, | |
txOptions | |
); | |
// Wait for 2 confirmations | |
const receipt = await tx.wait(2); | |
if (!receipt.status) { | |
throw new Error('Transaction failed'); | |
} | |
return receipt; | |
} catch (error: any) { | |
// Handle revert reasons | |
if (error.data) { | |
throw new Error(`Transaction reverted: ${error.data}`); | |
} | |
throw error; | |
} |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/core/src/core/providers.ts (3)
98-102
:⚠️ Potential issueAdd environment variable validation.
The code assumes environment variables will be present without validation.
120-123
:⚠️ Potential issueImprove gas limit handling.
The current implementation might lead to transaction failures if gas limit is not properly set.
126-132
:⚠️ Potential issueEnhance transaction execution and receipt handling.
The current implementation lacks proper transaction monitoring and receipt validation.
🧹 Nitpick comments (1)
packages/core/src/core/providers.ts (1)
87-92
: Consider making the network field more flexible.Instead of hardcoding "evm" as the only network type, consider using a union type to support potential future networks and maintain better type safety.
- network: "evm"; + network: "evm" | "arbitrum" | "optimism" | "polygon"; // Add other EVM-compatible networks as needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/core/providers.ts
(2 hunks)
🔇 Additional comments (1)
packages/core/src/core/providers.ts (1)
3-4
: LGTM! Required imports are correctly added.The imports for CoTTransaction type and ethers library are properly added to support the new EVM functionality.
} catch (error) { | ||
return error instanceof Error ? error : new Error("Unknown error occurred"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling specificity.
The current error handling loses valuable information by converting all errors to a generic message. Consider handling specific error types differently.
} catch (error) {
- return error instanceof Error ? error : new Error("Unknown error occurred");
+ if (error instanceof ethers.ContractTransactionExecutionError) {
+ return new Error(`Transaction execution failed: ${error.message}`);
+ } else if (error instanceof ethers.ContractTransactionRevertedError) {
+ return new Error(`Transaction reverted: ${error.message}`);
+ } else if (error instanceof ethers.ProviderError) {
+ return new Error(`Provider error: ${error.message}`);
+ } else {
+ return error instanceof Error ? error : new Error("Unknown error occurred");
+ }
📝 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.
} catch (error) { | |
return error instanceof Error ? error : new Error("Unknown error occurred"); | |
} | |
} catch (error) { | |
if (error instanceof ethers.ContractTransactionExecutionError) { | |
return new Error(`Transaction execution failed: ${error.message}`); | |
} else if (error instanceof ethers.ContractTransactionRevertedError) { | |
return new Error(`Transaction reverted: ${error.message}`); | |
} else if (error instanceof ethers.ProviderError) { | |
return new Error(`Provider error: ${error.message}`); | |
} else { | |
return error instanceof Error ? error : new Error("Unknown error occurred"); | |
} | |
} |
Adds basic EVM provider
(open to contributions)
Todo:
Summary by CodeRabbit
New Features
ethers
libraryDependencies
ethers
library version 6.13.5Documentation