AA-404 Adding stake manager #224
Conversation
| @@ -25,6 +28,43 @@ export class ValidationManagerRIP7560 implements IValidationManager { | |||
| // TODO | |||
| } | |||
|
|
|||
| async _getStakesInfo (operation: OperationBase): Promise<{ senderInfo: StakeInfo, paymasterInfo: StakeInfo, factoryInfo: StakeInfo }> { | |||
There was a problem hiding this comment.
Given that this method may need to make up to 3 (or 4 if we add aggregators later) roundtrip calls to the getDepositInfo function, maybe it would make sense to modify the function itself to look something like this:
function getDepositInfo(address[] accounts) external view returns (DepositInfo[] memory infos);
In ERC-4337 we didn't need to because we made the validation frame simulateValidation function return the struct of stake infos, but I don't think we need to use "simulations" in RIP-7560.
There was a problem hiding this comment.
The whole point was to not make changes to the StakeManager contract..
There was a problem hiding this comment.
it is still a better API to request them all in one call.
- we make multiple parallel calls to the node to request the value (using Promise.all)
- we can later use a helper contract that "batch" multiple calls to the nonce manager and return multiple values (e.g. our "BundlerHelper") - I'm not saying to add it now, but it leaves a room for it.
The point is that if the API now is for a single address, it is harder to batch it later.
| async _getStakesInfo (operation: OperationBase): Promise<{ senderInfo: StakeInfo, paymasterInfo: StakeInfo, factoryInfo: StakeInfo }> { | ||
| const senderTMI = await this.stakeManager.getDepositInfo(operation.sender) | ||
| const senderInfo = { | ||
| addr: operation.sender, |
There was a problem hiding this comment.
Not a good sign if you need to inject addr into returned value. Wouldn't it make sense for _getStakesInfo to return Map<string, StakeInfo>?
The parser would know the roles anyway, why keep them "named" in the return type ({ senderInfo: StakeInfo, paymasterInfo: StakeInfo, factoryInfo: StakeInfo }) ? UPD: I can see this is how this was done in ERC-4337. I still find it weird.
Also, anonymous return types become a problem very quickly IMO.
Also, maybe just make the getDepositInfo return a struct that already includes the address, just for completeness?
There was a problem hiding this comment.
the on-chain function return just the data. the bundler objcet returns also the entity name.
using a mapping makes an implicit assumption that the address identifiy the entity.
that is, not only you need the tx.paymaster value to query the paymaster's stake info, but you also rely on the fact it must differ from the other entities.
| export const stakeManagerSalt = '0x90d8084deab30c2a37c45e8d47f49f2f7965183cb6990a98943ef94940681de3' | ||
|
|
||
| export async function deployStakeManager (provider: JsonRpcProvider, signer = provider.getSigner()): Promise<StakeManager> { | ||
| const addr = await new DeterministicDeployer(provider, signer).deterministicDeploy(stakeManagerByteCode, stakeManagerSalt) |
There was a problem hiding this comment.
don't add salt, unless you actively want a specific "Vanity address".
There was a problem hiding this comment.
Maybe we will in the future. Left it there as a placeholder
There was a problem hiding this comment.
then default to null. don't default to useless value. the one value I know it won't have is this one, since it is for a very specific version of entrypoint.
| @@ -25,6 +28,43 @@ export class ValidationManagerRIP7560 implements IValidationManager { | |||
| // TODO | |||
| } | |||
|
|
|||
| async _getStakesInfo (operation: OperationBase): Promise<{ senderInfo: StakeInfo, paymasterInfo: StakeInfo, factoryInfo: StakeInfo }> { | |||
There was a problem hiding this comment.
it is still a better API to request them all in one call.
- we make multiple parallel calls to the node to request the value (using Promise.all)
- we can later use a helper contract that "batch" multiple calls to the nonce manager and return multiple values (e.g. our "BundlerHelper") - I'm not saying to add it now, but it leaves a room for it.
The point is that if the API now is for a single address, it is harder to batch it later.
| async _getStakesInfo (operation: OperationBase): Promise<{ senderInfo: StakeInfo, paymasterInfo: StakeInfo, factoryInfo: StakeInfo }> { | ||
| const senderTMI = await this.stakeManager.getDepositInfo(operation.sender) | ||
| const senderInfo = { | ||
| addr: operation.sender, |
There was a problem hiding this comment.
the on-chain function return just the data. the bundler objcet returns also the entity name.
using a mapping makes an implicit assumption that the address identifiy the entity.
that is, not only you need the tx.paymaster value to query the paymaster's stake info, but you also rely on the fact it must differ from the other entities.
No description provided.