Conversation
…packages into gj/gas_estimations_on_send
…packages into gj/no_from
yarn-project/aztec.js/src/contract/contract_function_interaction.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const deployOpts: DeployAccountOptions<InteractionWaitOptions> = { | ||
| from: AztecAddress.ZERO, | ||
| from: NO_FROM, |
There was a problem hiding this comment.
NO_FROM sounds so strange, isn't there some other way to name this hinting at its effects? maybe something like PAYLOAD_DEFINED_FROM (which is probably wronger and horribler, but I'm trying to convey my objection)
There was a problem hiding this comment.
seeing the changes to the docs, maybe CREATED_BY_TX
There was a problem hiding this comment.
Not in love with the naming too, I went for parallelism with NO_WAIT...
There was a problem hiding this comment.
It is weird, yes, but I'm also having trouble coming up with something better. Perhaps from: DIRECT_CALL? idk
There was a problem hiding this comment.
I agree that NO_FROM sounds strange. Now that Claude is back up, it suggested a few options:
- DIRECT or DIRECT_EXECUTION -- signals that the tx bypasses the account contract entrypoint
- NO_ACCOUNT -- clearer that you're opting out of account contract wrapping
- RAW -- short, implies no wrapping/mediation
- SELF -- the transaction acts on its own behalf without an intermediary account contract
- UNMEDIATED -- explicitly says "no account contract mediation"
- ENTRYPOINTLESS -- describes the mechanism being skipped (the account entrypoint)
- NO_SENDER -- closer to the domain: there's no "sender" account wrapping the call
And it even provided a ranking 😅
My ranking would be: DIRECT > NO_ACCOUNT > NO_SENDER > SELF > UNMEDIATED > ENTRYPOINTLESS > RAW > NO_FROM.
There was a problem hiding this comment.
Sorry but all of those are worse IMO xD
from is pretty generic, it has no concept of account (wallet just happens to make it so from is an account, but we had the exception of the multicall entrypoint...which is not one). The NO_WAIT parallel is also a pro in my book
| let overrides: SimulationOverrides | undefined; | ||
| let txRequest: TxExecutionRequest; | ||
| if (from === NO_FROM) { | ||
| const entrypoint = new DefaultEntrypoint(); | ||
| txRequest = await entrypoint.createTxExecutionRequest(finalExecutionPayload, feeOptions.gasSettings, chainInfo); | ||
| } else { | ||
| const useOverride = this.simulationMode === 'kernelless-override'; | ||
| let fromAccount: Account; | ||
| if (useOverride) { | ||
| const { account, instance, artifact } = await this.getFakeAccountDataFor(from); | ||
| fromAccount = account; | ||
| overrides = { | ||
| contracts: { [from.toString()]: { instance, artifact } }, | ||
| }; | ||
| } else { | ||
| fromAccount = await this.getAccountFromAddress(from); | ||
| } | ||
| const executionOptions: DefaultAccountEntrypointOptions = { | ||
| txNonce: Fr.random(), | ||
| cancellable: this.cancellableTransactions, | ||
| feePaymentMethodOptions: feeOptions.accountFeePaymentMethodOptions, | ||
| }; | ||
| txRequest = await fromAccount.createTxExecutionRequest( | ||
| finalExecutionPayload, | ||
| feeOptions.gasSettings, | ||
| chainInfo, | ||
| executionOptions, | ||
| ); | ||
| } |
There was a problem hiding this comment.
suggestion: extract this to an aux function that gives you { txRequest, overrides } so we can use early returns and avoid the fat else branch
There was a problem hiding this comment.
Since this is in itself a helper, I didn't want to muddy the waters further...I think it conveys "bare entrypoint vs. account contract well". But maybe I'm just too brain damaged by now
mverzilli
left a comment
There was a problem hiding this comment.
This makes a ton of sense and the implementation looks solid, my only concern is with NO_FROM naming given it's gonna be public API. I don't have a strong enough alternative I can propose so approving, maybe I would run this through people with a less close perspective of the codebase (eg devrel, Wonder, Ciara, etc)
…packages into gj/no_from
|
|
||
| const deployOpts: DeployAccountOptions<InteractionWaitOptions> = { | ||
| from: AztecAddress.ZERO, | ||
| from: NO_FROM, |
There was a problem hiding this comment.
It is weird, yes, but I'm also having trouble coming up with something better. Perhaps from: DIRECT_CALL? idk
| console.error( | ||
| "data.json not found. Run 'yarn data' first to generate proof data.", | ||
| ); |
There was a problem hiding this comment.
Is your formatter configured to fewer than 120 chars?
There was a problem hiding this comment.
the doc examples are...weird. I cannot get the ts server to behave with them and editing them does whatever. They also take forever to build...we have to talk to devrel about this
yarn-project/aztec.js/src/contract/contract_function_interaction.ts
Outdated
Show resolved
Hide resolved
| // Account contracts are always universally deployed (deployer = ZERO), | ||
| // but we still need to know the original `from` to detect self-deployment. |
There was a problem hiding this comment.
Why are they always universally deplyoed?
There was a problem hiding this comment.
Convention. Nothing prevents us from doing it differently
|
❌ Failed to cherry-pick to |
Getting rid of the tyranny of the
Accountentrypoint, and the terrible sentinel value ofAztecAddress.ZEROin thefromparameter.Now, when a tx is specified as being sent
from: NO_FROM, the wallet completely bypasses the account contract. It doesn't even use theMulticallEntrypointcontract, it'll just execute a single call usingDefaultEntrypoint. Since we can do wrapping at the app level, this means ANYTHING goes. Want to wrap 10 calls on a special sauce multicall? Go ahead. App-sponsored FPC with custom logic that must be the first on the chain? Knock yourself out!Even better news: this is thoroughly tested in our codebase thanks to account contract self-deployments. They're a great example of "I don't want to use an account contract as entrypoint" flow.
As an extra side effect, this completely deshrines the
MulticallEntrypointprotocol contract from the wallet (we only have the convenience now of it being already registered in every single wallet)