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 |
📝 WalkthroughWalkthroughThis 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
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 (
|
![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.
| 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); | ||
| } |
There was a problem hiding this comment.
Override required functions due to multiple inheritance
When inheriting from multiple contracts that implement the same functions, you need to override them to specify how to combine their behaviors. Specifically, you should override _beforeTokenTransfer and _burn to ensure compatibility with ERC721, ERC721Enumerable, and ERC721URIStorage.
Add the following overrides:
+ function _beforeTokenTransfer(
+ address from,
+ address to,
+ uint256 tokenId,
+ uint256 batchSize
+ ) internal override(ERC721, ERC721Enumerable) {
+ super._beforeTokenTransfer(from, to, tokenId, batchSize);
+ }
+ function _burn(uint256 tokenId) internal override(ERC721, ERC721URIStorage) {
+ super._burn(tokenId);
+ }Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| const nativeToken = | ||
| this.walletProvider.chains[params.chain].nativeCurrency.symbol; | ||
|
|
||
| let resp: TransferResponse = { | ||
| chain: params.chain, | ||
| txHash: "0x", | ||
| recipient: params.toAddress, | ||
| amount: "", | ||
| token: params.token ?? nativeToken, | ||
| }; | ||
|
|
||
| if (!params.token || params.token == nativeToken) { | ||
| // Native token transfer | ||
| let options: { gas?: bigint; gasPrice?: bigint; data?: Hex } = { | ||
| data: params.data, | ||
| }; | ||
| let value: bigint; | ||
| if (!params.amount) { | ||
| // Transfer all balance minus gas | ||
| const publicClient = this.walletProvider.getPublicClient( | ||
| params.chain | ||
| ); | ||
| const balance = await publicClient.getBalance({ | ||
| address: fromAddress, | ||
| }); | ||
|
|
||
| value = balance - this.DEFAULT_GAS_PRICE * 21000n; | ||
| options.gas = this.TRANSFER_GAS; | ||
| options.gasPrice = this.DEFAULT_GAS_PRICE; | ||
| } else { | ||
| value = parseEther(params.amount); | ||
| } | ||
|
|
||
| resp.amount = formatEther(value); | ||
| resp.txHash = await this.walletProvider.transfer( | ||
| params.chain, | ||
| params.toAddress, | ||
| value, | ||
| options | ||
| ); | ||
| } else { | ||
| // ERC20 token transfer | ||
| let tokenAddress = params.token; | ||
| if (!params.token.startsWith("0x")) { | ||
| tokenAddress = await this.walletProvider.getTokenAddress( | ||
| params.chain, | ||
| params.token | ||
| ); | ||
| } | ||
|
|
||
| const publicClient = this.walletProvider.getPublicClient( | ||
| params.chain | ||
| ); | ||
| const decimals = await publicClient.readContract({ | ||
| address: tokenAddress as `0x${string}`, | ||
| abi: ERC20Abi, | ||
| functionName: "decimals", | ||
| }); | ||
|
|
||
| let value: bigint; | ||
| if (!params.amount) { | ||
| value = await publicClient.readContract({ | ||
| address: tokenAddress as `0x${string}`, | ||
| abi: ERC20Abi, | ||
| functionName: "balanceOf", | ||
| args: [fromAddress], | ||
| }); | ||
| } else { | ||
| value = parseUnits(params.amount, decimals); | ||
| } | ||
|
|
||
| resp.amount = formatUnits(value, decimals); | ||
| resp.txHash = await this.walletProvider.transferERC20( | ||
| params.chain, | ||
| tokenAddress as `0x${string}`, | ||
| params.toAddress, | ||
| value | ||
| ); | ||
| } | ||
|
|
||
| if (!resp.txHash || resp.txHash == "0x") { | ||
| throw new Error("Get transaction hash failed"); | ||
| } | ||
|
|
||
| // wait for the transaction to be confirmed | ||
| const publicClient = this.walletProvider.getPublicClient( | ||
| params.chain | ||
| ); | ||
| await publicClient.waitForTransactionReceipt({ | ||
| hash: resp.txHash, | ||
| }); | ||
|
|
||
| return resp; | ||
| } catch (error) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary try/catch block in the transfer method.
The try/catch block that only rethrows the error without additional handling is unnecessary and can be removed to simplify the code.
Apply this diff to remove the redundant block:
- try {
// existing code
...
- } catch (error) {
- 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.
| try { | |
| const nativeToken = | |
| this.walletProvider.chains[params.chain].nativeCurrency.symbol; | |
| let resp: TransferResponse = { | |
| chain: params.chain, | |
| txHash: "0x", | |
| recipient: params.toAddress, | |
| amount: "", | |
| token: params.token ?? nativeToken, | |
| }; | |
| if (!params.token || params.token == nativeToken) { | |
| // Native token transfer | |
| let options: { gas?: bigint; gasPrice?: bigint; data?: Hex } = { | |
| data: params.data, | |
| }; | |
| let value: bigint; | |
| if (!params.amount) { | |
| // Transfer all balance minus gas | |
| const publicClient = this.walletProvider.getPublicClient( | |
| params.chain | |
| ); | |
| const balance = await publicClient.getBalance({ | |
| address: fromAddress, | |
| }); | |
| value = balance - this.DEFAULT_GAS_PRICE * 21000n; | |
| options.gas = this.TRANSFER_GAS; | |
| options.gasPrice = this.DEFAULT_GAS_PRICE; | |
| } else { | |
| value = parseEther(params.amount); | |
| } | |
| resp.amount = formatEther(value); | |
| resp.txHash = await this.walletProvider.transfer( | |
| params.chain, | |
| params.toAddress, | |
| value, | |
| options | |
| ); | |
| } else { | |
| // ERC20 token transfer | |
| let tokenAddress = params.token; | |
| if (!params.token.startsWith("0x")) { | |
| tokenAddress = await this.walletProvider.getTokenAddress( | |
| params.chain, | |
| params.token | |
| ); | |
| } | |
| const publicClient = this.walletProvider.getPublicClient( | |
| params.chain | |
| ); | |
| const decimals = await publicClient.readContract({ | |
| address: tokenAddress as `0x${string}`, | |
| abi: ERC20Abi, | |
| functionName: "decimals", | |
| }); | |
| let value: bigint; | |
| if (!params.amount) { | |
| value = await publicClient.readContract({ | |
| address: tokenAddress as `0x${string}`, | |
| abi: ERC20Abi, | |
| functionName: "balanceOf", | |
| args: [fromAddress], | |
| }); | |
| } else { | |
| value = parseUnits(params.amount, decimals); | |
| } | |
| resp.amount = formatUnits(value, decimals); | |
| resp.txHash = await this.walletProvider.transferERC20( | |
| params.chain, | |
| tokenAddress as `0x${string}`, | |
| params.toAddress, | |
| value | |
| ); | |
| } | |
| if (!resp.txHash || resp.txHash == "0x") { | |
| throw new Error("Get transaction hash failed"); | |
| } | |
| // wait for the transaction to be confirmed | |
| const publicClient = this.walletProvider.getPublicClient( | |
| params.chain | |
| ); | |
| await publicClient.waitForTransactionReceipt({ | |
| hash: resp.txHash, | |
| }); | |
| return resp; | |
| } catch (error) { | |
| throw error; | |
| } | |
| const nativeToken = | |
| this.walletProvider.chains[params.chain].nativeCurrency.symbol; | |
| let resp: TransferResponse = { | |
| chain: params.chain, | |
| txHash: "0x", | |
| recipient: params.toAddress, | |
| amount: "", | |
| token: params.token ?? nativeToken, | |
| }; | |
| if (!params.token || params.token == nativeToken) { | |
| // Native token transfer | |
| let options: { gas?: bigint; gasPrice?: bigint; data?: Hex } = { | |
| data: params.data, | |
| }; | |
| let value: bigint; | |
| if (!params.amount) { | |
| // Transfer all balance minus gas | |
| const publicClient = this.walletProvider.getPublicClient( | |
| params.chain | |
| ); | |
| const balance = await publicClient.getBalance({ | |
| address: fromAddress, | |
| }); | |
| value = balance - this.DEFAULT_GAS_PRICE * 21000n; | |
| options.gas = this.TRANSFER_GAS; | |
| options.gasPrice = this.DEFAULT_GAS_PRICE; | |
| } else { | |
| value = parseEther(params.amount); | |
| } | |
| resp.amount = formatEther(value); | |
| resp.txHash = await this.walletProvider.transfer( | |
| params.chain, | |
| params.toAddress, | |
| value, | |
| options | |
| ); | |
| } else { | |
| // ERC20 token transfer | |
| let tokenAddress = params.token; | |
| if (!params.token.startsWith("0x")) { | |
| tokenAddress = await this.walletProvider.getTokenAddress( | |
| params.chain, | |
| params.token | |
| ); | |
| } | |
| const publicClient = this.walletProvider.getPublicClient( | |
| params.chain | |
| ); | |
| const decimals = await publicClient.readContract({ | |
| address: tokenAddress as `0x${string}`, | |
| abi: ERC20Abi, | |
| functionName: "decimals", | |
| }); | |
| let value: bigint; | |
| if (!params.amount) { | |
| value = await publicClient.readContract({ | |
| address: tokenAddress as `0x${string}`, | |
| abi: ERC20Abi, | |
| functionName: "balanceOf", | |
| args: [fromAddress], | |
| }); | |
| } else { | |
| value = parseUnits(params.amount, decimals); | |
| } | |
| resp.amount = formatUnits(value, decimals); | |
| resp.txHash = await this.walletProvider.transferERC20( | |
| params.chain, | |
| tokenAddress as `0x${string}`, | |
| params.toAddress, | |
| value | |
| ); | |
| } | |
| if (!resp.txHash || resp.txHash == "0x") { | |
| throw new Error("Get transaction hash failed"); | |
| } | |
| // wait for the transaction to be confirmed | |
| const publicClient = this.walletProvider.getPublicClient( | |
| params.chain | |
| ); | |
| await publicClient.waitForTransactionReceipt({ | |
| hash: resp.txHash, | |
| }); | |
| return resp; |
🧰 Tools
🪛 Biome (1.9.4)
[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)
| string memory symbol, | ||
| uint8 decimalsValue, | ||
| uint256 initialSupply | ||
| ) ERC20(name, symbol) Ownable(msg.sender) { |
There was a problem hiding this comment.
Incorrect usage of Ownable constructor
The Ownable contract's constructor does not accept parameters. The call Ownable(msg.sender) is invalid. Ownable automatically sets the owner to msg.sender.
Apply this diff to fix the issue:
) ERC20(name, symbol) Ownable(msg.sender) {
_decimals = decimalsValue;
_mint(msg.sender, initialSupply);
}
+ ) ERC20(name, symbol) {
+ _decimals = decimalsValue;
+ _mint(msg.sender, initialSupply);
+ }Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol"; | ||
| import "@openzeppelin/contracts/access/Ownable.sol"; | ||
| import "@openzeppelin/contracts/token/ERC1155/extensions/ERC1155URIStorage.sol"; |
There was a problem hiding this comment.
Incorrect inheritance and Ownable constructor usage
The contract imports ERC1155URIStorage but doesn't inherit from it. Since _setURI is used, you should inherit from ERC1155URIStorage. Also, Ownable does not take parameters in its constructor.
Apply this diff to fix the issues:
import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/token/ERC1155/extensions/ERC1155URIStorage.sol";
-contract ERC1155Contract is ERC1155, Ownable {
+contract ERC1155Contract is ERC1155URIStorage, Ownable {
constructor(
string memory _name,
string memory _baseURI
- ) ERC1155(_baseURI) Ownable(msg.sender) {
+ ) ERC1155URIStorage(_baseURI) {
name = _name;
}
+ function uri(uint256 tokenId) public view override returns (string memory) {
+ return super.uri(tokenId);
+ }
}
Also applies to: 8-8
| token: "BNB", | ||
| }; | ||
| const resp = await ga.getBalance(input); | ||
| console.log("BNB balance", resp.balance); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test assertions and remove console.logs.
The tests should:
- Remove console.log statements
- Add proper assertions for balance values
- Include error cases (e.g., invalid tokens, addresses)
- console.log("BNB balance", resp.balance);
+ expect(resp.balance).toBeDefined();
+ expect(typeof resp.balance).toBe("string");
- console.log("USDC balance", resp.balance);
+ expect(resp.balance).toBeDefined();
+ expect(typeof resp.balance).toBe("string");
- console.log(
- "0x55d398326f99059ff775485246999027b3197955 balance",
- resp.balance
- );
+ expect(resp.balance).toBeDefined();
+ expect(typeof resp.balance).toBe("string");Also applies to: 42-42, 52-55
| it("should fetch balance", async () => { | ||
| let bal = await walletProvider.getWalletBalance(); | ||
| expect(bal).toEqual("0"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance balance test coverage.
The balance test only verifies a "0" balance. Add tests for:
- Non-zero balances
- Error cases (e.g., network issues)
- Different token types
| async validateAndNormalizeParams(params: GetBalanceParams): Promise<void> { | ||
| if (!params.address) { | ||
| params.address = this.walletProvider.getAddress(); | ||
| } else { | ||
| params.address = await this.walletProvider.formatAddress( | ||
| params.address | ||
| ); | ||
| } | ||
|
|
||
| if (!params.token || params.token === "") { | ||
| params.token = "BNB"; | ||
| } | ||
|
|
||
| if (params.chain != "bsc") { | ||
| // if token contract address is not provided, only BSC mainnet is supported | ||
| if (!params.token.startsWith("0x")) { | ||
| throw new Error( | ||
| "Only BSC mainnet is supported for querying balance by token symbol" | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Remove BSC-only restriction for token symbols
The validation should allow token symbols on opBNB since it's a fully supported chain in the plugin. Consider updating the condition to:
if (params.chain !== "bsc" && params.chain !== "opBNB") {🔗 Analysis chain
Consider supporting opBNB chain.
The chain validation currently restricts token symbol queries to BSC mainnet only. Consider extending support to opBNB since it's mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if opBNB support is implemented elsewhere
rg -g '!*.{md,json}' -i 'opbnb|op.*bnb'
Length of output: 6070
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