-
Notifications
You must be signed in to change notification settings - Fork 79
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
Bundler Package #163
Bundler Package #163
Changes from 5 commits
becac3d
81b818d
c1450f0
a7379dd
88af9bd
134e14b
387159d
8b08b5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
{ | ||
"name": "@biconomy/bundler-rpc", | ||
"version": "2.0.0", | ||
"description": "Biconomy Bundler Rpc to interact with any bundler", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Description should be Biconomy Bundler package to interact with any bundler node as per ERC4337 standard. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved |
||
"main": "./dist/src/index.js", | ||
"typings": "./dist/src/index.d.ts", | ||
"keywords": [ | ||
"Ethereum", | ||
"Gnosis", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove Gnosis keyword from this list. and add Bundler, Relayer, ERC4337, Gasless Transactions to the list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved |
||
"Biconomy", | ||
"SDK" | ||
], | ||
"scripts": { | ||
"unbuild": "rimraf dist *.tsbuildinfo", | ||
"build": "rimraf dist && tsc", | ||
"test": "yarn test:concurrently 'yarn test:run'", | ||
"test:file": "TS_NODE_PROJECT=../../tsconfig.json mocha -r ts-node/register --timeout 30000", | ||
"test:concurrently": "concurrently -k --success first 'yarn start:ganache > /dev/null'", | ||
"test:run": "yarn test:file tests/**/*.spec.ts", | ||
"start:ganache": "ganache -m 'direct buyer cliff train rice spirit census refuse glare expire innocent quote'", | ||
"format": "prettier --write \"{src,tests}/**/*.ts\"", | ||
"lint": "tslint -p tsconfig.json" | ||
}, | ||
"author": "talhamalik883 <talha.malik@biconomy.io>", | ||
"license": "MIT", | ||
"files": [ | ||
"dist/*", | ||
"README.md" | ||
], | ||
"publishConfig": { | ||
"access": "public" | ||
}, | ||
"devDependencies": { | ||
"@biconomy/core-types": "*", | ||
"@biconomy/common": "*" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import { IBundler } from "./interfaces/IBundler.interface"; | ||
import { UserOperation, ChainId } from '@biconomy/core-types' | ||
import { SendUserOpResponse, getUserOpGasPricesResponse } from "./types/Bundler.types" | ||
import { resolveProperties } from 'ethers/lib/utils' | ||
import { deepHexlify } from '@biconomy/common' | ||
import { HttpMethod, sendRequest } from './utils/httpRequests' | ||
|
||
export class Bundler implements IBundler { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add class level comments. This class implements IBundler interface. Implementation sends UserOperation to a bundler URL as per ERC4337 standard. Checkout the proposal for more details on Bundlers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved |
||
|
||
constructor(readonly bundlerUrl: string, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Convert the constructor params to a single object and add these parameters as property of that object. Check all other constructors and do the same changes there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved |
||
readonly entryPointAddress: string, | ||
readonly chainId: ChainId, | ||
readonly dappAPIKey: string) { | ||
} | ||
|
||
/** | ||
* | ||
* @param chainId | ||
* @description This function will fetch gasPrices from bundler | ||
* @returns | ||
*/ | ||
async getUserOpGasPrices(chainId: ChainId): Promise<getUserOpGasPricesResponse> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check the returned response and throw exception in case any expected value is missing or in wrong format. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inside function we are using sendRequest method and it throw exception whatever it gets from server There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type name should be UserOpGasPricesResponse get, put, delete should be used in function names and not in Types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rpc method name should be
as it return fields related to gas limit and gas price both. Similarly method name should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved |
||
const response: any = await sendRequest({ | ||
url: `${this.bundlerUrl}`, | ||
method: HttpMethod.Post, | ||
body: { | ||
method: 'eth_getUserOpGasPrices', | ||
params: [chainId], | ||
id: 1234, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why id is hardcoded here? It should be a unique number. We can also send the current timestamp as ID to ensure its unique for each request. Same goes for id used in sendUserOp method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure sachin. chirag wrote these calls before. I have added timestamp for it now |
||
jsonrpc: '2.0' | ||
} | ||
}) | ||
return response | ||
} | ||
/** | ||
* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Method level comment can be a bit more descriptive. Add the return type and description about what its doing with dappAPIKey. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. have removed the dapp key as its going to be the part of url |
||
* @param userOp | ||
* @description This function will send userOp to bundler | ||
* @returns | ||
*/ | ||
async sendUserOp(userOp: UserOperation): Promise<SendUserOpResponse> { | ||
const hexifiedUserOp = deepHexlify(await resolveProperties(userOp)) | ||
let params | ||
|
||
if (this.dappAPIKey && this.dappAPIKey !== '') { | ||
const metaData = { | ||
dappAPIKey: this.dappAPIKey | ||
} | ||
params = [hexifiedUserOp, this.entryPointAddress, this.chainId, metaData] | ||
} else { | ||
params = [hexifiedUserOp, this.entryPointAddress, this.chainId] | ||
} | ||
|
||
const response: SendUserOpResponse = await sendRequest({ | ||
url: `${this.bundlerUrl}`, | ||
method: HttpMethod.Post, | ||
body: { | ||
method: 'eth_sendUserOperation', | ||
params: params, | ||
id: 1234, | ||
jsonrpc: '2.0' | ||
} | ||
}) | ||
return response | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep the interface name as IBundler.ts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { SendUserOpResponse, getUserOpGasPricesResponse } from "../types/Bundler.types" | ||
import { ChainId,UserOperation } from '@biconomy/core-types' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Linting error. Give a space after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved |
||
|
||
export interface IBundler { | ||
getUserOpGasPrices(chainId: ChainId): Promise<getUserOpGasPricesResponse> | ||
sendUserOp(userOp: UserOperation): Promise<SendUserOpResponse> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
export type SendUserOpResponse = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These types should return only the object mentioned in data field. code and message are returned from the API call, and it should be handled inside the sdk methods. If code is anything other than 2xx then sdk should throw an exception with proper message so dapps can handle it at their end. Types name should be UserOpResponse = { UserOpGasFieldsResponse = { I removed the gasPrice field. Why there is need to gasPrice field? It's not used in creating UserOp There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. code and message are removed and types name are updated. gasPrice filed is just of type1 transaction as bundler just send gasPrice for non 1155 blockchain so we assign gasPrice value to maxFeePerGas and maxPriorityFeePerGas in userOP There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case of type1 transaction, maxFeePerGas and maxPriorityFeePerGas should have same value of gasPrice. This is mentioned in ERC as well. In userOp there is not gasPrice field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the method in EntryPoint.sol as well
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So bundler don't have to send gasPrice. Instead it should send same gasPrice value in both maxFeePerGas and maxPriorityFeePerGas |
||
code: number, | ||
message: string, | ||
data: { | ||
transactionOd: string, | ||
connectionUrl: string | ||
} | ||
} | ||
|
||
export type getUserOpGasPricesResponse = { | ||
code: number, | ||
message: string, | ||
data: { | ||
preVerificationGas: string, | ||
verificationGasLimit: string, | ||
callGasLimit: string, | ||
maxPriorityFeePerGas: string, | ||
maxFeePerGas: string, | ||
gasPrice: string | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,8 @@ import { | |
ITransactionResult | ||
} from '@biconomy/core-types' | ||
import { toTxResult } from '../../../utils' | ||
import { SmartWalletContractV100 as SmartWalletContract_TypeChain } from '../../../../typechain/src/ethers-v5/v1.0.0/SmartWalletContractV100' | ||
import { SmartWalletContractV100Interface } from '../../../../typechain/src/ethers-v5/v1.0.0/SmartWalletContractV100' | ||
import { SmartWalletContract_v1_0_0 as SmartWalletContract_TypeChain } from '../../../../typechain/src/ethers-v5/v1.0.0/SmartWalletContract_v1_0_0' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the reason for this change? Also, are we not going to use typechain and not ethers lib anymore two different questions ^ |
||
import { SmartWalletContract_v1_0_0Interface as SmartWalletContractV100Interface } from '../../../../typechain/src/ethers-v5/v1.0.0/SmartWalletContract_v1_0_0' | ||
import { Interface } from 'ethers/lib/utils' | ||
import { Contract } from '@ethersproject/contracts' | ||
import { BytesLike } from 'ethers' | ||
|
This file was deleted.
This file was deleted.
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.
Rename bundler-rpc to bundler.
Same goes for paymaster folder as well.
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.
resolved