Fix/state issue #180
Fix/state issue #180
Conversation
talhamalik883
changed the base branch from
master
to
fix/current-forward-flow-fixes-optimise
| @@ -67,6 +67,7 @@ export type SmartAccountState = { | |||
| isDeployed: boolean // chain specific | |||
| entryPointAddress: string // chain specific? | |||
| implementationAddress: string | |||
| factoryAddress: string | |||
There was a problem hiding this comment.
why did we have to add this? does this impact anywhere else when this is being used, since it's not optional
There was a problem hiding this comment.
The initializeContracts function now requires SmartAccountState instead of ISmartAccount. It's necessary to include the factoryAddress field in SmartAccountState, previously required in ISmartAccount. This change doesn't cause any issues because the factoryAddress is only set in the getAddress function, which has the needed information.
| @@ -46,6 +48,7 @@ import { JsonRpcProvider, Provider, Web3Provider } from '@ethersproject/provider | |||
| import { IRelayer, RestRelayer, FallbackRelayer, IFallbackRelayer } from '@biconomy/relayer' | |||
| import * as _ from 'lodash' | |||
| import TransactionManager, { | |||
| getSmartWalletFactoryContract, | |||
There was a problem hiding this comment.
getSmartWalletFactoryContract is directly exported from transactions package?
There was a problem hiding this comment.
We've taken out a step that used to get account information (getSmartAccountByOwner API) and we now get an address directly from the factory contract instead. To do this, we need to create a factory instance within the getAddress function, which is why we need to bring this function over from the transaction package.
| // // } | ||
|
|
||
| // if (isFallbackEnabled) { | ||
| // return this.sendFallbackTransaction(transactionDto) |
There was a problem hiding this comment.
let's remove this whole method also
this.sendFallbackTransaction
| @@ -8,5 +8,6 @@ export default TransactionManager | |||
| export * from './AccountUtils' | |||
| export * from './Execution' | |||
| export * from './MultiSend' | |||
| export * from './utils/FetchContractsInfo' | |||
| @@ -483,7 +483,7 @@ class TransactionManager { | |||
| const connectedWallet = smartAccountState.address | |||
| walletContract = walletContract.attach(connectedWallet) | |||
|
|
|||
| const isDeployed = smartAccountState.isDeployed | |||
| const isDeployed = await this.contractUtils.isDeployed(chainId, smartAccountState.address) | |||
There was a problem hiding this comment.
how is this reflected / needed?
There was a problem hiding this comment.
Previously, the batching function decided whether to fetch the nonce from the proxy or just use 0 based on the value of smartAccountState.isDeployed. If a new transaction was sent before resetting the SDK, right after deploying the wallet, this could lead to an invalid nonce error.
| @@ -52,7 +52,7 @@ class ContractUtils { | |||
| initializeContracts( | |||
| signer: Signer, | |||
| readProvider: ethers.providers.JsonRpcProvider, | |||
| walletInfo: ISmartAccount, | |||
| walletInfo: SmartAccountState, | |||
There was a problem hiding this comment.
what was the type earlier?
There was a problem hiding this comment.
ISmartAccount same as returned by getSmartAccountByOwner. This function called initializeContracts is set up using data we get from getSmartAccountByOwner api consumption.
This PR has following changes