-
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
Feat: Biconomy Paymaster #193
Conversation
…g context in specific classes only
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.
I have reviewed the code and added comments suggesting areas for improvement
tokenPaymasterRequest: BiconomyTokenPaymasterRequest | ||
): Promise<Partial<UserOperation>> { | ||
let batchTo: any = [] | ||
let batchValue: any = [] |
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 variable should have proper type instead of any
let batchTo: string = []
let batchValue: string = [] // can be string or BigNumber
let batchData: 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.
just updated types. do check in the file again
let batchData: any = [] | ||
let newCallData = userOp.callData | ||
Logger.log('received information about fee token address and quote ', tokenPaymasterRequest) | ||
const feeTokenAddress = tokenPaymasterRequest.feeQuote.tokenAddress |
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.
A validation function should be present here that validates the 'paymasterRequest' object. This validation includes checks for undefined and nullish values, as well as address validation, for instance, 'tokenAddress' and 'spender'
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.
I can validate token address and spender address for not being zero. what else?
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.
like below
if (!feeTokenAddress || feeTokenAddress == ethers.constants.AddressZero) {
Logger.log('Invalid or missing token address')
// Could possibly throw
return userOp
}
if (!userOp.callData || userOp.callData == '0x') { | ||
return userOp | ||
} | ||
const decodedDataSmartWallet = iFaceSmartWallet.parseTransaction({ |
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 try/catch block. If parseTransaction is provided with invalid data, it can throw an exception.
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.
whole execution is under try catch now
|
||
newCallData = this.getExecuteBatchCallData(batchTo, batchValue, batchData) | ||
} | ||
} |
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.
There should be an 'else' clause for the case of an unidentified method that throws an exception.
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.
where exactly?
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.
did not get this..
this is already checked before this code flow
if (smartWalletExecFunctionName === 'executeCall') {
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.
@talhamalik883 check this when you can
} | ||
} | ||
], // As per current API | ||
id: 4337, |
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.
@tomarsachin2271 suggested using 'currentTimeStamp' as a unique ID for each request. You can utilize the 'getTimestampInSeconds()' function from the common package to generate a timestamp to use as the ID.
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.
yup done
return { feeQuotes, tokenPaymasterAddress: '' } | ||
} | ||
} else { | ||
// REVIEW |
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.
throw exception here saying Failed to fetch feeQuote information
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.
working on it
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.
done
} | ||
} catch (error) { | ||
Logger.error("can't query fee quotes: ", error) | ||
// REVIEW |
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.
throw exception here saying Failed to fetch feeQuote information
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.
hmm I am making some changes in httpRequest utils. post that will update error handling here and will tag you to review again.
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.
done @talhamalik883 do check updated code and httpRequests file in commons
body: { | ||
method: 'pm_sponsorUserOperation', | ||
params: [userOp, paymasterServiceData], | ||
id: 1234, |
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.
replace id value with currentTimeStamp
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.
done
): Promise<Transaction> | ||
getPaymasterFeeQuotesOrData( | ||
userOp: Partial<UserOperation>, | ||
paymasterServiceData: FeeQuotesOrDataDto |
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.
change paymasterServiceData to feeQuoteInfo or feeQuoteOrData
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.
it could actually be just account information (for sponsorship mode) or feeQuote specific info like preferredToken
so really hard to decide the name and kept generic paymasterServiceData
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.
base on type, i guess using feeQuoteOrData would be better
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.
then i have to call it feeQuoteOrDataDto (basically camel case of dto name)
better is paymasterServiceData
Description
WIP
includes
BiconomyPaymaster (hybrid) with rpc calls pm_sponsorUserOperation and pm_getFeeQuotesOrData
Check calculations, error handling, types handling
Added method buildTokenPaymasterUserOp in the account package
hybrid paymaster class has method to checkTokenApproval and build transaction
for Biconomy account API above method modifies userOp with approval for asked token
BiconomySmartAccount -> buildPaymasterUserOp -> getTokenApprovalRequest
/////
need to review types
///
Testing scripts are updated here
yarn run smartAccount mintWithBtpm
yarn run smartAccount mint
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: