feat: add plugin-bnb to support BNB chain #2278
Conversation
pythonberg1997
changed the title
|
@shakkernerd could you take a look of this PR before the author moving forward? |
Okay, will do. |
|
We have I can you see you've got a deploy action, do you mind adding that action to The exception to continuing this will be if this implementation is targeting something completely different and if that will be a pain to implement in |
|
UPDATE: Thanks, looking forward to this. |
Thanks for the quick response, we will make it ready ASAP |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive BNB chain plugin for the agent ecosystem, adding support for blockchain interactions on the Binance Smart Chain (BSC) and opBNB. The changes include new configuration variables, smart contract templates, action handlers for token transfers, swaps, staking, and bridging, along with a robust wallet provider and extensive type definitions. Changes
Possibly related PRs
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (22)
packages/plugin-bnb/src/utils/contracts.ts (1)
74-74: Use consistent logging methodsReplace
console.warnandconsole.errorwithelizaLogger.warnandelizaLogger.errorfor consistent logging throughout the application.Apply this diff:
- console.warn("Compilation warnings:", output.errors); + elizaLogger.warn("Compilation warnings:", output.errors);- console.error("Compilation failed:", error); + elizaLogger.error("Compilation failed:", error);Also applies to: 90-90
packages/plugin-bnb/src/actions/swap.ts (3)
73-75: Remove unnecessary try-catch blockThe
catchblock simply rethrows the error without adding any handling. Removing it will simplify the code while maintaining the same functionality.Apply this diff:
return resp; - } catch (error) { - throw error; - }🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
78-82: Enhance parameter validationCurrently, only the
chainparameter is validated. Consider adding checks foramount,fromToken, andtoTokento ensure they are valid and prevent unexpected errors.Update the validation method:
validateAndNormalizeParams(params: SwapParams): void { if (params.chain != "bsc") { throw new Error("Only BSC mainnet is supported"); } + if (!params.amount || Number(params.amount) <= 0) { + throw new Error("Amount must be a positive number"); + } + if (!params.fromToken || !params.toToken) { + throw new Error("Token addresses must be specified"); + } }
53-54: Set default slippage value if undefinedIf
params.slippageis undefined, the swap may fail due to missing slippage configuration. Set a default slippage value to ensure smooth execution.Update the code to set a default slippage of 0.5%:
options: { - slippage: params.slippage, + slippage: params.slippage ?? 0.005, // Default to 0.5% slippage order: "RECOMMENDED", },And ensure
slippageis handled inswapOptions:const swapOptions: SwapParams = { chain: content.chain, fromToken: content.inputToken, toToken: content.outputToken, amount: content.amount, - slippage: content.slippage, + slippage: content.slippage ?? 0.005, // Default to 0.5% if undefined };Also applies to: 119-124
packages/plugin-bnb/src/actions/faucet.ts (2)
38-104: Simplify thefaucetmethod by avoiding manual Promise construction.The use of
new Promisein thefaucetmethod can be avoided. Refactoring to useasync/awaitwith event listeners will make the code cleaner and more maintainable.
98-102: Consider making the timeout duration configurable.A fixed 15-second timeout may not suffice under poor network conditions. Allowing the timeout duration to be configurable can improve the user experience.
packages/plugin-bnb/src/actions/deploy.ts (1)
137-139: Eliminate redundantcatchblock indeployERC721.The
catchblock merely rethrows the error without any additional handling. Removing it will streamline the method.Apply this diff:
try { const args = [name, symbol, baseURI]; const contractAddress = await this.deployContract( chain, "ERC721Contract", args ); return { address: contractAddress, }; - } catch (error) { - throw error; - }🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
packages/plugin-bnb/src/actions/stake.ts (1)
44-55: Remove unnecessary try/catch block in stake methodThe
try/catchblock in thestakemethod rethrows the caught error without adding any additional handling. It's redundant and can be removed to simplify the code.Apply this diff to remove the redundant
try/catchblock:- try { const actions = { deposit: async () => await this.doDeposit(params.amount!), withdraw: async () => await this.doWithdraw(params.amount), claim: async () => await this.doClaim(), }; const resp = await actions[params.action](); return { response: resp }; - } catch (error) { - throw error; - }🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
packages/plugin-bnb/src/actions/bridge.ts (1)
55-316: Eliminate redundant try/catch block in bridge methodThe
try/catchblock in thebridgemethod simply rethrows the error without additional processing. Removing it will streamline the code.Apply this diff to remove the unnecessary
try/catchblock:- try { // ... existing code ... return resp; - } catch (error) { - throw error; - }🧰 Tools
🪛 Biome (1.9.4)
[error] 315-315: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
packages/plugin-bnb/src/types/index.ts (3)
13-19: Maketokenandamountrequired inTransferParamsIn
TransferParams, bothtokenandamountare optional. Typically, a transfer action requires specifying the token and amount to transfer. Making these parameters mandatory can prevent errors due to missing values.
21-27: Set a default value forslippageinSwapParamsThe
slippageparameter is optional inSwapParams. Setting a reasonable default value can enhance user experience and prevent failed swaps due to unspecified slippage tolerance.
94-1222: Externalize large ABI definitionsThe ABI definitions are hardcoded in the TypeScript file. To improve maintainability and reduce clutter, consider loading these ABIs from external JSON files.
packages/plugin-bnb/src/index.ts (2)
26-26: Move implementation notes to documentation.The comment about bridge limitations should be moved to the README.md or API documentation for better visibility and maintenance.
17-18: Enhance plugin metadata.Consider adding more descriptive metadata about supported features and chains.
- name: "bnb", - description: "BNB Smart Chain integration plugin", + name: "bnb", + description: "BNB Smart Chain (BSC) and opBNB integration plugin supporting transfers, swaps, staking, bridging, and token deployments",packages/plugin-bnb/src/templates/index.ts (2)
8-8: Apply DRY principle to chain validation.Chain validation is repeated across templates. Consider extracting it to a shared constant:
+const SUPPORTED_CHAINS = ["bsc", "bscTestnet", "opBNB", "opBNBTestnet"] as const; +const SUPPORTED_CHAINS_TEXT = `Must be one of ${JSON.stringify(SUPPORTED_CHAINS)}. Default is "bsc".`; -Chain to execute on. Must be one of ["bsc", "bscTestnet", "opBNB", "opBNBTestnet"]. Default is "bsc". +Chain to execute on. ${SUPPORTED_CHAINS_TEXT}Also applies to: 31-31, 58-58, 85-86, 113-113
174-182: Improve type definitions in contract template.The JSON response types should be more precise:
"chain": SUPPORTED_CHAINS, "contractType": "ERC20" | "ERC721" | "ERC1155", "name": string, - "symbol": string, - "decimals": number, - "totalSupply": string, - "baseURI": string + "symbol": string | null, // null for ERC1155 + "decimals": number | null, // null for ERC721/ERC1155 + "totalSupply": string | null, // null for ERC721/ERC1155 + "baseURI": string | null // null for ERC20agent/src/index.ts (1)
944-948: Simplify the conditional check using optional chaining.The condition for including the BNB plugin can be simplified.
- getSecret(character, "BNB_PRIVATE_KEY") || - (getSecret(character, "BNB_PUBLIC_KEY") && - getSecret(character, "BNB_PUBLIC_KEY")?.startsWith("0x")) - ? bnbPlugin - : null, + getSecret(character, "BNB_PRIVATE_KEY") || + getSecret(character, "BNB_PUBLIC_KEY")?.startsWith("0x") + ? bnbPlugin + : null,🧰 Tools
🪛 Biome (1.9.4)
[error] 945-946: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/plugin-bnb/README.md (2)
13-16: Enhance security guidance in configuration section.Consider adding:
- Warning about secure storage of private keys
- Recommendation to use environment variables over direct values
- Note about not committing .env file
121-121: Add rate limit values to faucet documentation.Specify the exact rate limit values to improve clarity.
packages/plugin-bnb/src/actions/getBalance.ts (2)
82-84: Remove unnecessary try-catch block.The try-catch block that only rethrows the error adds no value and can be removed.
- try { let resp: GetBalanceResponse = { chain, address: address!, }; // ... rest of the code ... return resp; - } catch (error) { - throw error; - }🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
31-85: Consider caching token decimals.The implementation looks good but could benefit from caching token decimals to reduce RPC calls for frequently queried tokens.
🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
.env.example (1)
498-503: Add validation hints for environment variables.Consider adding format hints for the variables:
- BNB_PRIVATE_KEY should be a hex string
- BNB_PUBLIC_KEY should be a 0x-prefixed address
- Provider URLs should be valid HTTP(S) URLs
-BNB_PRIVATE_KEY= # BNB chain private key -BNB_PUBLIC_KEY= # BNB-smart-chain public key (address) -BSC_PROVIDER_URL= # BNB-smart-chain rpc url -OPBNB_PROVIDER_URL= # OPBNB rpc url +BNB_PRIVATE_KEY= # BNB chain private key (hex string without 0x prefix) +BNB_PUBLIC_KEY= # BNB-smart-chain public key (0x-prefixed address) +BSC_PROVIDER_URL= # BNB-smart-chain RPC URL (e.g., https://bsc-dataseed.binance.org) +OPBNB_PROVIDER_URL= # opBNB RPC URL (e.g., https://opbnb-mainnet-rpc.bnbchain.org)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.env.example(1 hunks).gitignore(1 hunks)agent/package.json(1 hunks)agent/src/index.ts(2 hunks)client/src/lib/info.json(1 hunks)packages/plugin-bnb/README.md(1 hunks)packages/plugin-bnb/package.json(1 hunks)packages/plugin-bnb/src/actions/bridge.ts(1 hunks)packages/plugin-bnb/src/actions/deploy.ts(1 hunks)packages/plugin-bnb/src/actions/faucet.ts(1 hunks)packages/plugin-bnb/src/actions/getBalance.ts(1 hunks)packages/plugin-bnb/src/actions/stake.ts(1 hunks)packages/plugin-bnb/src/actions/swap.ts(1 hunks)packages/plugin-bnb/src/actions/transfer.ts(1 hunks)packages/plugin-bnb/src/contracts/Erc1155Contract.sol(1 hunks)packages/plugin-bnb/src/contracts/Erc20Contract.sol(1 hunks)packages/plugin-bnb/src/contracts/Erc721Contract.sol(1 hunks)packages/plugin-bnb/src/index.ts(1 hunks)packages/plugin-bnb/src/providers/wallet.ts(1 hunks)packages/plugin-bnb/src/templates/index.ts(1 hunks)packages/plugin-bnb/src/tests/getBalance.test.ts(1 hunks)packages/plugin-bnb/src/tests/wallet.test.ts(1 hunks)packages/plugin-bnb/src/types/index.ts(1 hunks)packages/plugin-bnb/src/utils/contracts.ts(1 hunks)packages/plugin-bnb/tsconfig.json(1 hunks)packages/plugin-bnb/tsup.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- client/src/lib/info.json
- packages/plugin-bnb/tsconfig.json
- packages/plugin-bnb/tsup.config.ts
🧰 Additional context used
📓 Learnings (1)
packages/plugin-bnb/src/actions/swap.ts (3)
Learnt from: B1boid
PR: elizaOS/eliza#2332
File: packages/plugin-evm/src/actions/swap.ts:0-0
Timestamp: 2025-01-17T19:36:49.663Z
Learning: The Bebop API URL (https://api.bebop.xyz) in the SwapAction is intentionally hardcoded as the system only operates in the production environment.
Learnt from: B1boid
PR: elizaOS/eliza#2332
File: packages/plugin-evm/src/actions/swap.ts:0-0
Timestamp: 2025-01-17T19:29:52.172Z
Learning: When comparing token amounts from different DEX aggregators in the SwapAction, use BigInt for direct comparison as the amounts are already normalized to the same decimal precision.
Learnt from: B1boid
PR: elizaOS/eliza#2332
File: packages/plugin-evm/src/actions/swap.ts:230-242
Timestamp: 2025-01-17T19:27:59.723Z
Learning: The transaction configuration in EVM plugins uses unimplemented KZG functions and undefined chain parameter as a type compatibility pattern with viem's transaction type. This is an accepted pattern across the codebase.
🪛 Biome (1.9.4)
agent/src/index.ts
[error] 945-946: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/plugin-bnb/src/actions/swap.ts
[error] 74-74: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
packages/plugin-bnb/src/actions/deploy.ts
[error] 138-138: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
packages/plugin-bnb/src/actions/bridge.ts
[error] 315-315: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
packages/plugin-bnb/src/actions/transfer.ts
[error] 140-140: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
packages/plugin-bnb/src/actions/stake.ts
[error] 53-53: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
packages/plugin-bnb/src/actions/getBalance.ts
[error] 83-83: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🔇 Additional comments (7)
packages/plugin-bnb/src/contracts/Erc721Contract.sol (2)
16-16:⚠️ Potential issueCorrect the
Ownableconstructor callThe
Ownablecontract constructor does not accept parameters. Removemsg.senderto properly initialize theOwnablecontract.Apply this diff to fix the constructor:
- ) ERC721(name, symbol) Ownable(msg.sender) { + ) ERC721(name, symbol) Ownable() {Likely invalid or redundant comment.
55-68:⚠️ Potential issueRemove overrides of non-existent functions
The functions
_updateand_increaseBalancedo not exist inERC721orERC721Enumerablefrom OpenZeppelin. Overriding these functions may cause compilation errors.Apply this diff to remove the incorrect overrides:
- function _update( - address to, - uint256 tokenId, - address auth - ) internal override(ERC721, ERC721Enumerable) returns (address) { - return super._update(to, tokenId, auth); - } - function _increaseBalance( - address account, - uint128 value - ) internal override(ERC721, ERC721Enumerable) { - super._increaseBalance(account, value); - }Likely invalid or redundant comment.
packages/plugin-bnb/src/providers/wallet.ts (1)
1-355: Well-structured implementation of WalletProviderThe
WalletProviderclass and its methods are comprehensive, correctly implementing the wallet functionalities for BNB chain interactions. The code is clean and follows best practices.agent/src/index.ts (1)
56-56: LGTM!The import follows the established pattern for plugin imports.
packages/plugin-bnb/package.json (1)
13-15: Review version constraints for security.The following dependencies use caret ranges which may introduce breaking changes:
@openzeppelin/contracts@types/node✅ Verification successful
Dependencies and version constraints are appropriate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known vulnerabilities in dependencies npm audit --package-lock-only # Check latest versions npm view @openzeppelin/contracts version npm view @types/node versionLength of output: 468
agent/package.json (1)
44-44: LGTM!The dependency addition follows the established pattern for workspace plugins.
packages/plugin-bnb/src/actions/getBalance.ts (1)
134-283: Well-structured action definition with comprehensive examples!The action definition includes clear examples covering various use cases: checking native token balance, ERC20 token balance by symbol, and by contract address.
# Conflicts: # .gitignore # agent/package.json # agent/src/index.ts # pnpm-lock.yaml
|
@shakkernerd please help review, thank you. |
|
We can resolve these two files' conflicts later, right before the final merge, as they tend to conflict frequently. |
# Conflicts: # agent/package.json # pnpm-lock.yaml
Okay, I'll look into it. |
Relates to
N/A (No specific issue or ticket linked)
Risks
Low
Background
We are developers from NodeReal, a leading blockchain infrastructure provider delivering comprehensive one-stop blockchain solutions. As a key contributor to BNB Chain and enthusiastic adopters of AI technology, we are excited to contribute the BNB plugin to Eliza.
What does this PR do?
This PR adds a new plugin to Eliza for BNB Chain support. With this plugin, users can perform various operations on BNB Chain, including getBalance, transfer, swap, stake, faucet, bridge, and deploy ERC20/ERC721/ERC1155 tokens.
What kind of change is this?
Features (non-breaking change which adds functionality)
Testing
Where should a reviewer start?
From
packages/plugin-bnbfolder.Detailed testing steps
Discord username
pythonberg
Summary by CodeRabbit
Here are the release notes for this pull request:
New Features
Configuration Updates
.env.examplewith BNB-related keys and provider URLsVersion Update
0.1.8+build.1to0.1.9-alpha.1Development Improvements