ethereumjs-monorepo icon indicating copy to clipboard operation
ethereumjs-monorepo copied to clipboard

Breaking/Deprecation Tracking Issue

Open holgerd77 opened this issue 1 year ago • 5 comments

Tracking issue for breaking changes and deprecation tasks for an eventual round of breaking releases (roughly August/September 2024). A release round would follow the v7 releases from #2945 (Summer 2023).

Note: We have done so many additional breaking changes since this write-up (which are already completed) that it's not worth to continue to maintain or update this list. There are some remaining tasks to be extracted (in some updated form), otherwise treat this TODO list as rather outdated.

General

  • [x] Revisit new/option methods/properties in exposed interfaces (StateManger,...) and see if to make things mandatory, if design is generally optimal or things should move,...
  • [ ] Look into BigInt <-> Bytes conversion for certain edge cases https://github.com/ethereumjs/ethereumjs-monorepo/issues/3180

Browser / Bundle Size / Performance

  • [x] Remove last Node polyfill needs (event emitter, other?), see e.g. #3330
  • [x] Integrate Tree Shaking Optimizations, see #3446 overview and sub tasks

@ethereumjs/util

  • [ ] Generally look through the bytes module and think what consistent (and eventually more length-strict) strategy we want to persue there, also regarding to the new length-specific helpers newly introduced by @gabrocheleau
  • [x] Go through the bytes module, and related functions that accept both PrefixedHexString | string as types, and restrict the type to only accept PrefixedHexString. Addressed by https://github.com/ethereumjs/ethereumjs-monorepo/pull/3510

@ethereumjs/common

  • [x] Get rid of networkId (superseeded by chainId), see #3046 (might need validation) #3513
  • [ ] #3037
  • [x] Make the StateManager interface's getAppliedKey method introduced by #3143 non-optional.

@ethereumjs/trie

  • [ ] Make trie option in genWithdrawalsTrieRoot()and genTransactionTrieRoot() mandatory to always have the eventual WASM crypto passed to the trie, see https://github.com/ethereumjs/ethereumjs-monorepo/pull/3192/files#r1448613272 for context
  • [x] #3231 remove old stream method, eventually rename to generic, short name and remove dependency (remember to close #3218 once the old stream method is deprecated) #3519
  • [x] Deal with deprecated methods - particularly around proofs - and align remaining inconsistent naming (like some names with createFromProof() vs fromProof() (likely better), also have a look at StateManager in this context) ~~- [ ] Try to revive Web Streams API implementation (needed to be removed for browser compatibility reasons) in some alternative way (external scripts + generally remove all hard-wired stream functionality? Needs to be decided/discussed), also affects SM, see #3231 and #3280 for context~~ not applicable with #3519
  • [x] Clean up utility methods - at a minimum unify nibblesToBytes and nibblestoBytes https://github.com/ethereumjs/ethereumjs-monorepo/issues/3521

@ethereumjs/statemanager

  • [x] Make getContractCode not throw https://github.com/ethereumjs/ethereumjs-monorepo/issues/3479

@ethereumjs/tx

  • [ ] Finally refactor this +/- 27 insanity for v in ecsign (somewhere else?) in a clean way!!!, see also e.g. #1607, #1597, #1961, #1845
  • [ ] Rename internal field representations to match the JSON RPC names: "coinbase field should be miner, uncleHash should be sha3Uncles, transactionsTrie should be transactionsRoot and receiptTrie should be receiptsRoot" as discussed with "tomorrowforsure.eth" in the chat (eventually look at VM.runBuild(), VM.runTx() receipt names as well, also eventually analog for Block properties (so at least a check))?
  • [ ] Rename tx.getBaseFee() to something which does not use the term "base fee". (Also change the error in VM). See: https://github.com/ethereumjs/ethereumjs-monorepo/issues/3216#issuecomment-1916689056

@ethereumjs/block

  • [ ] Rename validation-hiding getValidationErrors() (and the like) in tx, block to something like isValidWithErrors()
  • [ ] Fix order of deposits in main block constructors (move before opts) - see e.g. https://github.com/ethereumjs/ethereumjs-monorepo/pull/3303 (executionWitness can/should likely remain where it is until Verkle is integrated)
  • [ ] Maybe we get the calcDifficultyFromHeader and cliqueSigner options out of the BlockOptions or at least better hide/encapsule this legacy stuff? Not sure.

@ethereumjs/verkle

  • [x] See if we can come up with a generic tree interface for both Trie and Verkle which we can put in Common (as we do for StateManager e.g.) which abstracts away to a greater extend the need for a Verkle/Trie distinction (at least for some places/usages, might also turn out to be only a subset of the functionality)

holgerd77 avatar Jan 08 '24 08:01 holgerd77

To align the JSON RPC names we should consider the official (?) spec for the names: https://github.com/ethereum/execution-apis/tree/main/src/eth. (And while we are doing it, check all, so also block, or eth_call methods)

jochem-brouwer avatar Jan 27 '24 08:01 jochem-brouwer

There is something which I keep noticing, and keep getting confused about:

https://github.com/ethereumjs/ethereumjs-monorepo/blob/d8f7544bf86231781b178f936459d96ddaf3e3a1/packages/vm/src/runTx.ts#L264-L267

This is NOT the base fee as in EIP-1559, but it is the "base fee" of the transaction. This is the cost of the tx (21000) plus the data fee (depends on calldata) and extra fee if this is a CREATE transaction. However we also have this tx.getBaseFee() method (which returns this). We should definitely rename this to something else, because this is super confusing. (Will add to tx)

jochem-brouwer avatar Jan 30 '24 11:01 jochem-brouwer

Linking https://github.com/ethereumjs/ethereumjs-monorepo/issues/3479 (make getContractStorage not throw for non-existing accounts)

jochem-brouwer avatar Jul 01 '24 09:07 jochem-brouwer

There is something which I keep noticing, and keep getting confused about:

https://github.com/ethereumjs/ethereumjs-monorepo/blob/d8f7544bf86231781b178f936459d96ddaf3e3a1/packages/vm/src/runTx.ts#L264-L267

This is NOT the base fee as in EIP-1559, but it is the "base fee" of the transaction. This is the cost of the tx (21000) plus the data fee (depends on calldata) and extra fee if this is a CREATE transaction. However we also have this tx.getBaseFee() method (which returns this). We should definitely rename this to something else, because this is super confusing. (Will add to tx)

I think this is what EIPs usually refer to as intrinsic gas or something like that. That name isn't any more obvious but at least if we use that term, it would align us with broader core dev usage.

acolytec3 avatar Jul 19 '24 00:07 acolytec3

Ok, I will open a PR to make these terms more clear :) I'll check the EIPs what terms they use but I think I indeed remember instrinsic gas.

jochem-brouwer avatar Jul 22 '24 13:07 jochem-brouwer