Skip to content
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.

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

feat: signature (transaction) types #15

Merged
merged 4 commits into from
Jun 14, 2023
Merged

feat: signature (transaction) types #15

merged 4 commits into from
Jun 14, 2023

Conversation

mds1
Copy link
Owner

@mds1 mds1 commented Jun 13, 2023

Adds EIP-2718 transaction and signature types for mainnet, arbitrum, and optimism, along with a basic rendering of their diff. Showing more detailed info in the diff + style improvements will come in a future PR.

One thing currently implied is that both arbitrum and optimism both consider prefix byte 0x03 reserved and will not use it. Optimism's spec for the Deposited Transaction type suggests they'll avoid using that prefix byte:

Picking a high identifier minimizes the risk that the identifier will be used be claimed by another transaction type on the L1 chain in the future.

I couldn't explicitly find anything similar for Arbitrum, but given that their new transaction types also use high identifiers, I imagine they also don't plan to use it

@vercel
Copy link

vercel bot commented Jun 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
evm-diff ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2023 1:34am

Comment on lines +4 to +13
export type SignatureType = {
prefixByte: number;
description: string;
// The data that is RLP encoded and signed to generate a signed transaction.
signedData: string[] | undefined;
// Some signature types are used to sign transactions, others are used to sign data.
signs: 'transaction' | 'data' | undefined;
references: string[];
notes?: string[];
};

Choose a reason for hiding this comment

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

Would include hardforks: []string here, protocol specs change. And then when importing L1 mainnet tx types in L2 support lists, filter by supported L1 forks. When L1 updates, not all L2s update immediately. E.g. 1559 wasn't on L2 everywhere the instant it was a mainnet L1 tx type.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yea I was thinking about this too. Since it applies to other things (e.g. opcodes) I'm going to add this more broadly in a future PR. Now tracking in #16

const isEqual = JSON.stringify(baseSigType) === JSON.stringify(targetSigType);
const showSigType = !isEqual || !onlyShowDiff;

return (

Choose a reason for hiding this comment

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

Would be nice to show the hardfork range info here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Also tracking in #16

references: [
txTypeDocs,
'https://github.com/OffchainLabs/go-ethereum/blob/master/core/types/transaction.go#L53',
'https://github.com/OffchainLabs/go-ethereum/blob/master/core/types/arb_types.go#L387-L390',
Copy link

@protolambda protolambda Jun 13, 2023

Choose a reason for hiding this comment

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

These line numbers are already broken, the file changed yesterday. I recommend pinning links to a commit or release tag.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Heh was wondering if these files were stable enough to link to master directly to avoid clicking a permalink and having to wonder if that link is actually up to date. Seems the answer is no, so back to permalinks 😅

import { SignatureType } from '@/chains';
import { signatureTypes as mainnetSignatureTypes } from '@/chains/mainnet/signatureTypes';

const txTypeDocs = 'https://developer.arbitrum.io/arbos/geth#transaction-types';

Choose a reason for hiding this comment

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

Thought I noticed a gap, and there is indeed a tx type 103 (0x067) that was removed but existed previously. Not sure if it ended up in a mainnet deployment. It was the "ArbitrumWrappedTxType". Worth documenting maybe?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah great find, do you have a link? Searched their issues/PRs/codebase for that name and number but didn't find it

For predeploys there's deprecated: bool field, seems worth adding the same for transaction types.

Arguably type 0 transactions should be considered deprecated too, since they're almost always worse. Only (contrived) exception I can think of is you have a bit less calldata to post on L1 with a type 0 tx since there's less gas params to specify, so it may be cheaper in some instances (e.g. sweeping an account where you don't care about the refund)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Tracking this in #17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants