Repository navigation
refactor(accounts)!: unify frame account and payer constructors - #55
nikhilkumar1612 wants to merge 3 commits into
Conversation
mariano-aguero
left a comment
There was a problem hiding this comment.
Thanks for unifying these constructors, the single entry point makes the account API much easier to follow.
| if (signatureIndex === -1) { | ||
| const entries = await getSignatureEntries(transaction) | ||
| prepared = { | ||
| ...transaction, | ||
| signatures: [...transaction.signatures, ...entries], | ||
| } | ||
| signatureIndex = findOwnerSignatureIndex(prepared, owner, 'last') | ||
| } |
There was a problem hiding this comment.
When no payer entry is found, this appends one after the sender has already signed. The sig-hash only elides signature bytes, so the entry list itself is hashed, and appending changes the hash the sender's signature was made over. signFrameTransaction runs the sender first and then the payer, and assertValidFrameTx does not recover signatures, so a transaction built with signatures: [] and a payer comes back with an invalid sender signature and no error. The old payer threw here. The sender default has the mirror problem at lines 110 to 117: prepending its entry invalidates a payer signature that is already present. Could we only insert when nothing has been signed yet, and throw otherwise so callers install entries through prepareFrameTransaction? The test "does not hardcode the payer signature index" still passes with this, and a test that signs sender plus payer from signatures: [] and recovers index 0 would pin it.
| if (signatureIndex === -1) { | |
| const entries = await getSignatureEntries(transaction) | |
| prepared = { | |
| ...transaction, | |
| signatures: [...transaction.signatures, ...entries], | |
| } | |
| signatureIndex = findOwnerSignatureIndex(prepared, owner, 'last') | |
| } | |
| if (signatureIndex === -1) { | |
| if (transaction.signatures.some((entry) => entry.signature !== '0x')) | |
| throw new FrameEncodeError( | |
| `frame payer ${await getAddress()} has no entry and other entries are already signed; install payer entries during preparation`, | |
| ) | |
| const entries = await getSignatureEntries(transaction) | |
| prepared = { | |
| ...transaction, | |
| signatures: [...transaction.signatures, ...entries], | |
| } | |
| signatureIndex = findOwnerSignatureIndex(prepared, owner, 'last') | |
| } |
| const signingOwner = ownerWithSigner(owner, parameters.sign) | ||
|
|
||
| async function assertPayFrame(transaction: FrameTransaction): Promise<void> { | ||
| const payer = await getAddress() |
There was a problem hiding this comment.
The payer reuses findOwnerSignatureIndex, which resolves a null signer to transaction.sender. If someone hands the payer a transaction whose sender is the payer itself, with a pay frame targeting it and a single implicit SECP256K1 entry, the default payer signs index 0, which is the sender's own validation entry. A sponsor that signs transactions it receives from users would then authorize SENDER frames spending its own balance. Copying signature bytes made this possible before too, but the new default does it directly. A cheap guard would be to reject isAddressEqual(transaction.sender, payer) in assertPayFrame, or to make the payer match only entries with an explicit signer, plus a test with sender === payer.
| /** Expected EIP-7702 delegate. Omit for a non-delegated account. */ | ||
| delegateAddress?: Address | undefined |
There was a problem hiding this comment.
delegateAddress is documented here and in DESIGN.md as the expected EIP-7702 delegate, which reads as if it were checked, but nothing compares it with the account's code. Its only effect is at line 93, where it switches the entry from an implicit to an explicit signer, and on the payer (line 25) it is never read at all. A caller who passes the wrong delegate gets no error. Could we either verify it against eth_getCode (0xef0100 || delegate) or rename it to describe what it does, such as an explicit-signer flag, and drop it from the payer parameters?
| const useImplicitSigner = | ||
| parameters.delegateAddress === undefined && | ||
| ownerSignatureScheme(owner) === 1 && | ||
| isAddressEqual(sender, owner.address) |
There was a problem hiding this comment.
These two conditions decide whether the wire entry carries signer: null or the owner's address, which changes the encoded bytes and the sig-hash, but no test reaches them with a secp256k1 owner. Deleting parameters.delegateAddress === undefined && or isAddressEqual(sender, owner.address) leaves the suite green: the P-256 tests pass delegateAddress but scheme 2 already forces an explicit signer, so it has no effect there. Two small tests with privateKeyToAccount, one with delegateAddress and one with address set to a different EOA, asserting that getSignatureEntries returns signer: owner.address, would cover both.
| parameters.getNonce ?? | ||
| (async (nonceParameters: GetFrameNonceParameters = {}) => | ||
| getFrameNonceSeq(client, { | ||
| address: await getAddress(), |
There was a problem hiding this comment.
The nonce default reads from getAddress(), which is right for a delegated account, but both nonce tests use a plain EOA where address and owner.address are the same. Replacing this with owner.address would pass every test, and for a P-256 owner it would read the nonce of the P-256 identity instead of the delegated EOA, producing a transaction the chain rejects. A test that builds an account with address: DELEGATED_EOA, records the transport requests, and asserts the nonce call uses DELEGATED_EOA would catch it.
| /** Create a payer account with overridable defaults for common payment policies. */ | ||
| export async function toFramePayerAccount< | ||
| const implementation extends FramePayerAccountImplementation, | ||
| owner extends FrameAccountOwner, |
There was a problem hiding this comment.
Making owner a required FrameAccountOwner means a contract payer with no key can no longer be built without a placeholder owner, even though FramePayerAccountImplementation still allows owner: undefined and the README says contract payers use this constructor. The previous const implementation generic also preserved the caller's own implementation type in the return value, and ToFramePayerAccountParameters and ToFrameAccountParameters changed their type parameters. Since this is already a major release, could we add these to the breaking-change list in the description, or keep owner optional when every callback is overridden?
| if (sign === undefined) return owner | ||
| if (owner.type === 'p256') return { ...owner, sign } | ||
| return { ...owner, sign } as Account |
There was a problem hiding this comment.
With the new sign override, the secp256k1 path never checks that the adapter signed with owner.address: signSecp256k1Entry uses the returned bytes as is, while the P-256 path verifies against the public key. If a wallet switches accounts or the adapter falls back to a prefixed eth_sign, the user signs, the transaction is assembled and broadcast, and only the chain rejects it. Wrapping the adapter here to call recoverAddress({ hash, signature }) and throw unless it matches owner.address would fail fast. The P-256 branch on line 13 also has no test; replacing it with return owner passes the suite.
| frame.mode === 1 && | ||
| frame.flags === 0x1 && | ||
| frame.target !== null && | ||
| isAddressEqual(frame.target, payer), |
There was a problem hiding this comment.
Every payer test builds the payer with defaults where address equals owner.address, so ignoring parameters.address, or comparing the frame target with owner.address here, would not fail any test. The two assertPayFrame calls also cover for each other, so either one can be deleted without a failure. A test with toFramePayerAccount({ owner, address: OTHER }) that signs when the pay frame targets OTHER and rejects when it targets owner.address, plus a direct getSignatureEntries call without a pay frame, would pin both.
| const signFrameTransaction = | ||
| parameters.signFrameTransaction ?? | ||
| (async (transaction: FrameTransaction) => { | ||
| const sender = await getAddress() |
There was a problem hiding this comment.
The defaults call getAddress() again on every nonce read, entry build and sign, even though the address is resolved once at line 127. With a custom getAddress that hits RPC, such as a counterfactual address from a factory, that is one extra round trip per nonce key plus two or three per send, and if it ever returns something different the defaults validate against a different address than account.address, which prepareFrameTransaction used as the sender. Resolving const address = await getAddress() before building the defaults and using it inside them would avoid both. The payer has the same pattern at lines 57 and 97.
| signature is produced, so both signatures commit to the same transaction hash. Custom | ||
| contract payers can be defined with `toFramePayerAccount` to provide their PAY calldata, | ||
| signature entries, and signing behavior. | ||
| signature. Both entries are installed during preparation, before either signature is |
There was a problem hiding this comment.
This sentence still calls the payer's entry "the protocol-reserved payment signature", while DESIGN.md now says signing locates the payer entry rather than assuming a fixed index, and the old code required index 1. If the default code really reserves index 1 for the pay frame, the new lookup should enforce it; if it does not, the README wording should drop "protocol-reserved". Could we confirm against ethrex and align both docs with the code?
|
thanks for the review @mariano-aguero will take a look at them in a while |
BREAKING CHANGE: Specialized EOA, P-256, and EOA payer constructors are replaced by toFrameAccount and toFramePayerAccount.
mariano-aguero
left a comment
There was a problem hiding this comment.
Thanks for the quick and thorough follow-up, nikhilkumar1612, the earlier points are all addressed and the new tests read really well.
| const signFrameTransaction = | ||
| parameters.signFrameTransaction ?? | ||
| (async (transaction: FrameTransaction) => { | ||
| await assertPayFrame(transaction) |
There was a problem hiding this comment.
The default payer signs any transaction that has a payment-only VERIFY frame targeting it, including one whose sender is the payer itself, and the test at test/accounts.test.ts:200 now pins that case as supported. When sender and payer are the same address, the payer entry { scheme: 1, signer: payer, msg: '0x' } is a full sig-hash signature whose resolved signer is also the sender, so a sponsor that signs transactions built by someone else could end up approving SENDER frames that execute as the payer (a transfer of its own balance, for instance), as long as the sender's default-code VERIFY accepts any entry that resolves to it. A self-paying EOA does not need a pay frame, since self_verify with scope 0x3 already covers it, so I would suggest having the default policy refuse isAddressEqual(transaction.sender, address) here and in getSignatureEntries, turning the test at line 200 into a rejection test, and adding a line to DESIGN.md saying the default payer applies no other policy (no cap on maxCost or on frame contents), so anyone running it as a sponsor knows to override signFrameTransaction.
| frame.mode === 1 && | ||
| frame.flags === 0x1 && |
There was a problem hiding this comment.
No test exercises these two conditions: every test that reaches the payer sets the targeted frame to mode 1 with flags 0x1, and the negative test at test/accounts.test.ts:228 fails only because the first frame of GOLDEN_TX has a null target. Deleting frame.mode === 1 or frame.flags === 0x1 keeps the suite green, and either change would let the default payer sign a transaction where the frame naming it is a SENDER frame or a VERIFY frame with scope 0x3, which is what this guard is there to prevent. Two small negative cases next to the existing one, a frame targeting the payer with flags 0x3 and another with mode 2, would pin both.
| await assertPayFrame(transaction) | ||
|
|
||
| let prepared = transaction | ||
| let signatureIndex = findOwnerSignatureIndex(prepared, owner, 'last', { |
There was a problem hiding this comment.
The 'last' direction here and 'first' in toFrameAccount.ts:190 are what keep sender and payer on separate entries when one key controls both, for example a delegated sender with explicit signer K and a payer owned by K, which gives [{1, K}, {1, K}]. Both entries hash the same, so if either direction were flipped both accounts would sign the same index and leave the other one as 0x, and no test covers that layout: the sender == payer test at test/accounts.test.ts:200 uses [null, K], where explicitSigner already rules out index 0, and test/signFrameTransaction.test.ts uses distinct owners. A case that runs signFrameTransaction(account, tx, { payer }) with a shared owner and asserts both indices recover to K would make the direction load-bearing in the suite.
| parameters.getSignatureEntries ?? | ||
| (async (transaction: FrameTransaction) => { | ||
| await assertPayFrame(transaction) | ||
| return [ownerSignatureEntry(owner, owner.address)] |
There was a problem hiding this comment.
For a P-256 owner this default produces a scheme-2 entry with an explicit signer, but the only P-256 payer test (test/accounts.test.ts:286) overrides getSignatureEntries and signFrameTransaction and calls signFrameSignature(tx, 1, owner) itself, so it checks its own callbacks rather than the defaults the PR description lists as covered. Breaking the scheme, the explicit signer, or the explicitSigner lookup for scheme 2 would all go unnoticed. Building toFramePayerAccount({ owner: p256Owner, address: OTHER }) with no overrides, a pay frame targeting OTHER and signatures: [], then asserting the entry is { scheme: 2, signer: owner.address } and that it verifies with P256.verify, would cover the real path.
| const code = (await client.request({ | ||
| method: 'eth_getCode', | ||
| params: [address, 'latest'], | ||
| })) as Hex | ||
| const expected = `0xef0100${parameters.delegateAddress.slice(2)}`.toLowerCase() | ||
| if (code.toLowerCase() !== expected) |
There was a problem hiding this comment.
Thanks for adding this check, it is what gives delegateAddress its meaning. Right now, though, both mocks that serve eth_getCode (test/p256.test.ts:159 and test/accounts.test.ts:448) return the delegation code whatever the params are, and the negative test returns 0x. Querying owner.address instead of address, or relaxing the comparison to something like code.endsWith(delegate), would pass every test. Recording the request in the P-256 test and asserting params: [DELEGATED_EOA, 'latest'], plus a negative case with code that ends in the delegate address but lacks the 0xef0100 prefix, would pin both halves.
| (async () => { | ||
| const useImplicitSigner = | ||
| parameters.delegateAddress === undefined && | ||
| ownerSignatureScheme(owner) === 1 && |
There was a problem hiding this comment.
This condition decides whether a P-256 owner whose identity equals the sender gets the explicit signer that DESIGN.md requires for P256 entries, but no test reaches it: the P-256 tests either pass a distinct address or bring their own entry (test/p256.test.ts:217). Removing ownerSignatureScheme(owner) === 1 would emit signer: null for that owner, the lookup would still match because a null signer resolves to the sender, and nothing would fail. Starting the test at test/p256.test.ts:217 from signatures: [] and asserting signatures[0].signer === signer.address would cover it.
| directly, normalizes `s` low, and encodes `r || s || qx || qy`. Passing `address` separately | ||
| represents a delegated EOA controlled by a different signer. When `delegateAddress` is | ||
| provided, construction verifies that `eth_getCode(address)` is exactly | ||
| `0xef0100 || delegateAddress`. Account-specific validation or signature policies |
There was a problem hiding this comment.
This paragraph describes delegateAddress only as a construction-time eth_getCode check, but toFrameAccount.ts:174 also uses it to switch a secp256k1 owner whose address equals the sender from signer: null to an explicit signer, which changes the entry bytes, the sig-hash and the billed calldata. The test at test/accounts.test.ts:442 shows the switch is intended, so since DESIGN.md is the binding reference, it would help to add a sentence here saying a delegated sender always gets explicit signer metadata.
| const address = await (parameters.getAddress?.() ?? | ||
| Promise.resolve(unresolvedAddress!)) |
There was a problem hiding this comment.
When a caller passes both address and getAddress, getAddress wins and address is dropped silently, while the JSDoc on address only says it defaults to owner.address. Now that getAddress runs exactly once at construction it is equivalent to address: await fn(), so either documenting the precedence or rejecting both at once (as the ownerless parameter types already do with their union) would avoid a quiet mismatch. The same applies to toFramePayerAccount.ts:83.
| const expected = `0xef0100${parameters.delegateAddress.slice(2)}`.toLowerCase() | ||
| if (code.toLowerCase() !== expected) | ||
| throw new FrameEncodeError( | ||
| `frame account ${address} delegates with ${code}, expected ${expected}`, |
There was a problem hiding this comment.
If address turns out to be a regular contract rather than a delegated EOA, eth_getCode returns its whole runtime code, up to about 48 KB of hex, and all of it ends up in this error message and in whatever logs it reaches. A delegation designator is exactly 48 characters as hex, so truncating anything longer keeps the useful case readable.
| `frame account ${address} delegates with ${code}, expected ${expected}`, | |
| `frame account ${address} delegates with ${code.length > 48 ? `${code.slice(0, 48)}...` : code}, expected ${expected}`, |
| if (code === undefined || !code.toLowerCase().startsWith('0xef0100')) | ||
| if (code === undefined || !/^0xef0100[0-9a-fA-F]{40}$/.test(code)) | ||
| throw new Error(`sender ${sender} does not have an EIP-7702 delegation`) | ||
| const delegateAddress = getAddress(`0x${code.slice(8)}`) |
There was a problem hiding this comment.
Since delegateAddress is derived from the same getCode result that toFrameAccount fetches again, the new constructor check can never fail in this script, and it adds a second eth_getCode round trip. As this script is the reference for the delegated P-256 flow, reading the expected implementation from an env variable (next to P256_SIGNER_IDENTITY) and dropping the local regex check would show the parameter doing its job.
Reject ambiguous addresses and unsafe self-paying payer defaults, strengthen delegation validation, and add regression coverage for P-256 and shared-owner signing
|
thanks for checking again @mariano-aguero fixes are in place, can you take a look again 🙏 |
What this changes
Replaces the specialized
toEoaFrameAccount,toP256FrameAccount, andtoEoaFramePayerAccountconstructors with the unifiedtoFrameAccountandtoFramePayerAccountAPIs.The unified constructors infer defaults from the supplied owner while allowing account-specific behavior to be overridden.
Why
The specialized constructors duplicated address resolution, nonce handling, signature-entry construction, and signing behavior.
A unified API gives applications one consistent integration point for:
privateKeyToAccountThis also makes it possible to add future authorization mechanisms without introducing another top-level constructor for every account type.
How the new behaviour is pinned
Which oracle covers this change, and why it could actually fail:
frame_tx_wire_tests.rsmaxCost, or thegasUseddecomposition)The account tests independently recover generated secp256k1 signatures and verify P-256 signatures. Existing wire-format, captured-chain, signature, gas, and receipt-oracle tests continue to pass unchanged.
Load-bearing rules
Did this touch anything under "Rules that are not style preferences" in
CONTRIBUTING.md, such as a derived gas constant, a golden vector, a captured fixture, the module import order, orencodeFrameTxvalidating?Checks
Only tick what you ran.
bun run testbun run typecheckbun run build && bunx @arethetypeswrong/cli --pack .(the build passed;@arethetypeswrong/clihas not been run)bun run test:live(optional, hits the public endpoint, not a gate)Additional verification:
scripts/temp.ts.Release impact
The commit history sets the version, so say which this is:
docs,chore,test,ci,refactor,style,perf,build)fix)feat)feat!or aBREAKING CHANGE:footer)This removes the following public exports:
toEoaFrameAccounttoP256FrameAccounttoEoaFramePayerAccountMigration examples: