feat: wallet routes public view calls directly through node for simulations#20245
feat: wallet routes public view calls directly through node for simulations#20245
Conversation
yarn-project/pxe/src/contract_function_simulator/contract_function_simulator.ts
Outdated
Show resolved
Hide resolved
| * Simulates a transaction, optimizing public static calls by running them directly | ||
| * on the node while sending the remaining calls through the standard PXE path. | ||
| * Return values from both paths are merged back in original call order. | ||
| * @param executionPayload - The execution payload to simulate. | ||
| * @param opts - Simulation options (from address, fee settings, etc.). | ||
| * @returns The merged simulation result. | ||
| */ |
There was a problem hiding this comment.
The "optimizing public static calls by running... etc" seems like an extremely ad-hoc comment that makes sense when reviewing this PR but feels odd to highlight in general. Also it leaks implementation details that will make it probably a bit harder to grok to an occasional dev
There was a problem hiding this comment.
Mmm, I think it's important in this case. Furthermore, we should instruct app devs to try and run public static calls first on their payloads (or even on a dedicated payload!) so they can take advantage of the optimization. This is why I debated having the optimization be in the wallet or providing a standalone function, discoverability.
| true /* simulatePublic */, | ||
| opts?.skipTxValidation, | ||
| opts?.skipFeeEnforcement ?? true, | ||
| const { publicStaticCalls, otherCalls } = extractOptimizablePublicStaticCalls(executionPayload); |
There was a problem hiding this comment.
might feel like I'm contradicting myself but while I think we shouldn't mention the optimization in the js-doc of this function, the function body could use some comments to explain why we're going into the trouble of writing it like this
nchamo
left a comment
There was a problem hiding this comment.
Left some small suggestions
mverzilli
left a comment
There was a problem hiding this comment.
Left a bunch of comments but nothing blocker. Perhaps the main concern would be making sure that we're not leaking functions from utils that are too implementation oriented, which then people will start using, which then we might need to maintain/deprecate/document/etc
e4507d3 to
04c9a63
Compare
nchamo
left a comment
There was a problem hiding this comment.
Great work, really like how it looks now 🔥
yarn-project/aztec.js/src/contract/contract_function_interaction.ts
Outdated
Show resolved
Hide resolved
| const balanceOf = await makeFunctionCall(FunctionType.PUBLIC, true, 'balanceOf'); | ||
| const totalSupply = await makeFunctionCall(FunctionType.PUBLIC, true, 'totalSupply'); | ||
| const transfer = await makeFunctionCall(FunctionType.PRIVATE, false, 'transfer'); |
There was a problem hiding this comment.
mega-nit: I had to keep going to makeFunctionCall signature to remind myself what true and false mean here. would suggest to make it either a named param or even split in two auxes: (makeStaticFunctionCall and makeNonStaticFunctionCall)... or perhaps an enum (akin to FunctionType.PRIVATE etc)
| const transfer = await makeFunctionCall(FunctionType.PRIVATE, false, 'transfer'); | ||
| const payload = new ExecutionPayload([balanceOf, totalSupply, transfer], [], []); | ||
|
|
||
| const optimizedRv0 = new NestedProcessReturnValues([new Fr(100)]); |
There was a problem hiding this comment.
NestedProcessReturnValues is a perplexing name but maybe it's just I'm not familiar with it 😅
There was a problem hiding this comment.
This can be refactored when we start pulling callstack data from the node
| } | ||
|
|
||
| /** | ||
| * Simulates calls through the standard PXE path (account entrypoint). |
There was a problem hiding this comment.
can we maybe comment a bit more on the alternative/s to this? or what it is useful for?
There was a problem hiding this comment.
I'm refactoring this on a followup PR that creates an @aztec/wallets package, moves test-wallet to e2e, etc
There was a problem hiding this comment.
I guess this is the dual of simulateViaNode, so maybe referring to it would be enough
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
| throw publicOutput.revertReason; | ||
| } | ||
|
|
||
| return new TxSimulationResult(privateResult, provingResult.publicInputs, publicOutput, undefined); |
There was a problem hiding this comment.
is privateResult garbage here? wondering if we should allow some falsy type to signal that, or at least comment somewhere
There was a problem hiding this comment.
It's essentially garbage, but I think it's better to keep the shape consistent for downstream consumers. We can probably iterate on this when we work on adding more stuff (eg offchain effects) to the returns of simulate and send
…ations Optimization suggested by @olehmisar and implemented slightly differently here. Reading public information from wallets is way faster AND parallelizable if we go directly to the node. This PR assembles a fake Tx object with only PublicCalls, that the node will accept and simulate in batches of 32. Co-authored-by: Charlie <5764343+charlielye@users.noreply.github.com> Co-authored-by: Gregorio Juliana <gregojquiros@gmail.com> Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar> Co-authored-by: benesjan <13470840+benesjan@users.noreply.github.com> Co-authored-by: mverzilli <651693+mverzilli@users.noreply.github.com> Co-authored-by: mverzilli <martin@aztec-labs.com> Co-authored-by: thunkar <gregojquiros@gmail.com>
80a0c19 to
8b6a0b4
Compare
| from: await AztecAddress.random(), | ||
| to: await AztecAddress.random(), | ||
| amount: BigInt(amount), | ||
| it('splits a mixed payload into optimized and entrypoint paths and merges results', async () => { |
There was a problem hiding this comment.
we could maybe add some more test cases:
- all optimizable
- none optimizable
- some optimizable after some non-optimizable (if I understand correctly in this case we don't want to optimize?)
|
This is now causing me a bit of a problem in this PR as I am working on displaying the debug logs from public function simulations because:
Was there some fundamental reason why we didn't want |
Optimization suggested by @olehmisar and implemented slightly differently here.
Reading public information from wallets is way faster AND parallelizable if we go directly to the node. This PR assembles a fake Tx object with only PublicCalls, that the node will accept and simulate in batches of 32.