-
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
Conversation
packages/bundler-rpc/package.json
Outdated
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
packages/bundler-rpc/package.json
Outdated
"typings": "./dist/src/index.d.ts", | ||
"keywords": [ | ||
"Ethereum", | ||
"Gnosis", |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
packages/bundler-rpc/package.json
Outdated
{ | ||
"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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
packages/bundler-rpc/src/Bundler.ts
Outdated
|
||
export class Bundler implements IBundler { | ||
|
||
constructor(readonly bundlerUrl: string, |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
packages/bundler-rpc/src/Bundler.ts
Outdated
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
packages/bundler-rpc/src/Bundler.ts
Outdated
* @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 comment
The reason will be displayed to describe this comment to others. Learn more.
This rpc method name should be
eth_getUserOpGasFields
as it return fields related to gas limit and gas price both.
Similarly method name should be getUserOpGasFields
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
packages/bundler-rpc/src/Bundler.ts
Outdated
return response | ||
} | ||
/** | ||
* |
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.
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 comment
The 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
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.
Keep the interface name as IBundler.ts
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
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Linting error. Give a space after ChainId,
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
@@ -0,0 +1,21 @@ | |||
export type SendUserOpResponse = { |
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.
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 = {
userOpHash: string,
wait: <mention function type here that return Promise>
}
UserOpGasFieldsResponse = {
preVerificationGas: string,
verificationGasLimit: string,
callGasLimit: string,
maxPriorityFeePerGas: string,
maxFeePerGas: string
}
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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
See the method in EntryPoint.sol as well
function getUserOpGasPrice(MemoryUserOp memory mUserOp) internal view returns (uint256) {
unchecked {
uint256 maxFeePerGas = mUserOp.maxFeePerGas;
uint256 maxPriorityFeePerGas = mUserOp.maxPriorityFeePerGas;
if (maxFeePerGas == maxPriorityFeePerGas) {
//legacy mode (for networks that don't support basefee opcode)
return maxFeePerGas;
}
return min(maxFeePerGas, maxPriorityFeePerGas + block.basefee);
}
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.
See the method in EntryPoint.sol as well
function getUserOpGasPrice(MemoryUserOp memory mUserOp) internal view returns (uint256) { unchecked { uint256 maxFeePerGas = mUserOp.maxFeePerGas; uint256 maxPriorityFeePerGas = mUserOp.maxPriorityFeePerGas; if (maxFeePerGas == maxPriorityFeePerGas) { //legacy mode (for networks that don't support basefee opcode) return maxFeePerGas; } return min(maxFeePerGas, maxPriorityFeePerGas + block.basefee); }
So bundler don't have to send gasPrice. Instead it should send same gasPrice value in both maxFeePerGas and maxPriorityFeePerGas
Some of the changes that i did in bundler package in my latest work is also update in this branch |
@@ -0,0 +1,7 @@ | |||
import { UserOpResponse, UserOpGasPricesResponse } from "../types/Types" |
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 UserOpGasPricesResponse
to UserOpGasFieldsResponse
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
packages/bundler/src/types/Types.ts
Outdated
entryPointAddress: string | ||
} | ||
|
||
export type UserOpResponse = { | ||
data: { | ||
transactionOd: string, |
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.
Typo here
It should be transactionId
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
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.
does this change also include forward scrapping?
seeing lot of files being deleted
@@ -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 comment
The 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 ^
/* eslint-disable @typescript-eslint/no-explicit-any */ | ||
interface HttpRequest { | ||
url: string | ||
method: HttpMethod | ||
body?: Record<string, any> | ||
headers?: object | ||
} | ||
|
||
export async function sendRequest<T>({ url, method, body, headers = {} }: HttpRequest): Promise<T> { |
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.
instead of renaming relayer to bundler would this not be a fresh package from scratch?
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.
looks good
This PR includes bundler package integration in modular sdk code