Skip to content

fix: tempo e2e #4188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jxom merged 15 commits into from
Dec 21, 2025
Merged

fix: tempo e2e #4188

jxom merged 15 commits into from
Dec 21, 2025

Conversation

@jenpaff
Copy link
Collaborator

@jenpaff jenpaff commented Dec 19, 2025 •
edited
Loading

we made recent changes in the gas schedule for access keys, and therefore this test fails

this PR adds 2 new fields for gas estimate

running tests tempo e2e tests locally works

VITE_TEMPO_ENV=devnet pnpm test --project tempo e2e
 
 Test Files  1 passed (1)
      Tests  30 passed (30)
   Start at  18:52:31
   Duration  35.08s

@changeset-bot
Copy link

changeset-bot bot commented Dec 19, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: fce446c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
viem Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 19, 2025 •
edited
Loading

Open in StackBlitz

npm i https://pkg.pr.new/viem@4188

commit: b3409d2

@jenpaff jenpaff changed the title fix tempo e2e tests fix tempo e2e sendTransaction tests Dec 19, 2025
@tmm tmm marked this pull request as ready for review December 19, 2025 18:22
tmm
tmm reviewed Dec 19, 2025
View reviewed changes
src/tempo/Formatters.ts Outdated
Comment on lines 128 to 154
const keyAuthorization = rpc.keyAuthorization
? (() => {
const keyAuthorization = {
...rpc.keyAuthorization,
// FIXME(ox): chainId not serialized in nested object
chainId:
rpc.keyAuthorization.chainId === '0x' ||
rpc.keyAuthorization.chainId === '0x0'
? 0
: parseInt(rpc.keyAuthorization.chainId || '0x0', 16),
}
// FIXME(ox): expects `preHash` not `prehash`
if (
keyAuthorization.signature &&
keyAuthorization.signature.type === 'p256' &&
'prehash' in keyAuthorization.signature
) {
const { prehash, ...rest } = keyAuthorization.signature
keyAuthorization.signature = {
...rest,
// @ts-expect-error
preHash: prehash,
}
}
return keyAuthorization
})()
: undefined
Copy link
Member

@tmm tmm Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fix this upstream in Ox

jxom
jxom reviewed Dec 19, 2025
View reviewed changes
src/tempo/Formatters.ts Outdated
? 0
: parseInt(rpc.keyAuthorization.chainId || '0x0', 16),
}
// FIXME(ox): expects `preHash` not `prehash`
Copy link
Member

@jxom jxom Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true

Copy link
Member

@tmm tmm Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol well tests only pass with this probably because it gets omitted then 😓

Copy link
Member

@tmm tmm Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec says preHash and tempo codebase uses pre_hash (which maps to camelized preHash, not lowercase prehash, assuming the same behavior as other fields)

Copy link
Member

@jxom jxom Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, I read it as "ox expects preHash", not "rpc expects preHash" :P

jxom
jxom reviewed Dec 19, 2025
View reviewed changes
src/tempo/Formatters.ts Outdated
chainId:
rpc.keyAuthorization.chainId === '0x' ||
rpc.keyAuthorization.chainId === '0x0'
? 0
Copy link
Member

@jxom jxom Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RPC doesn’t serialize chain ids?

Copy link
Member

@tmm tmm Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests fail without it

jxom
jxom reviewed Dec 19, 2025
View reviewed changes
src/tempo/Formatters.ts Outdated
: undefined
const keyAuthorization = rpc.keyAuthorization
? (() => {
const keyAuthorization = {
Copy link
Member

@jxom jxom Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think we need to coerce this at all as TransactionRequest.toRpc already handles key auths. My question is that does the RPC expect a different format for chain ids and prehash naming? Guess we can update the prehash naming in Ox though

@jxom
Copy link
Member

jxom commented Dec 19, 2025 •
edited
Loading

Just updated Ox for the preHash change. Also added temporary prepareTransactionRequest hook for 2D nonce gas until eth_fillTransaction/eth_estimateGas is fixed for 2D nonces. Though, Tempo devnet tests are failing because the RPC is currently down.

@jxom jxom force-pushed the jenpaff/update-devnet-url branch from b3409d2 to fce446c Compare December 21, 2025 21:29
@jxom jxom changed the title fix tempo e2e sendTransaction tests fix: tempo e2e Dec 21, 2025
@jxom jxom merged commit 0e589b2 into main Dec 21, 2025
22 of 35 checks passed
@jxom jxom deleted the jenpaff/update-devnet-url branch December 21, 2025 21:30
@github-actions github-actions bot mentioned this pull request Dec 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmm tmm tmm left review comments

@jxom jxom jxom left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jenpaff @jxom @tmm