Object.freeze(this) in a constructor restricts class extension

[cgewecke]

#923, #907

Object.freeze(this) is called at the end of the Transaction, Block, and BlockHeader constructors.

Surprisingly, if you extend any of these classes and add properties, the derived class's constructor throws.

Example

const Transaction = require('@ethereumjs/tx').Transaction

class SpecialTransaction extends Transaction {
  constructor(){
    super()
    this.special = true;
  }
}

new SpecialTransaction()

Throws

TypeError: Cannot add property special, object is not extensible

Am guessing the freeze locks the outputs of the new generators but I wonder if locating it in the constructor might be too restrictive for users who want to use these libs as a base for other work.

At ganache-core, for example, Transaction is extended here.

[holgerd77]

@cgewecke That's a great find, thanks for going into this! I have pushed #941, I think this should be a good way to handle it. People get still the increased security properties of freezing by default, but can also deactivate if necessary (I guess there also might be more customization cases where this is needed). If they do they have to explicitly make the decision and are also guided to re-enable by the comment from the option.

When reading the code from Ganache you referenced I was a bit tempted to also remove the readonly properties since I assume that these might cause problems as well when going into the Ganache setup deeper and I also assumed that the Object.freeze would safe-guard here as an alternative to readonly when activated. I assumed here that - testing with ts-node - an attempted property change on a frozen object should throw, so e.g. when deactivating readonly on gasPrice and running the following code:

import { Transaction } from './src'
import { BN, toBuffer } from 'ethereumjs-util'
let tx = Transaction.fromTxData({})
tx.gasPrice = new BN(toBuffer('0x0005'))
console.log(tx.gasPrice)

But this in fact does NOT throw but just keeps the value and outputs <BN: 0>, so this seems to be highly dangerous in some contexts and should be handled with care.

My suggestion now after these first experiences would be that we keep this readonly for now and see if people get along. If there are too many complaints it should be possible to loosen this in a non-breakable manner.

Another thing I noticed: subclassing is not particularly user friendly with this new structure, since one has to pass all the tx main constructor parameter values to the super() call. Not sure if there is some optimization possible, maybe this whole thing (subclassing, extending) should generally be given some thought how this works in practice, also with calling the helper functions. //cc @ryanio

Note: along with the PR mentioned I also fixed a significant bug with the options not being passed to the main constructor on the block static factory methods.

[holgerd77]

(not sure would it makes sense to pass these data options here in as a dict to the main constructors of block and tx? Or are we loosing here too much with this on the other end?)

[jochem-brouwer]

Wow - this Object.freeze stuff is extremely weird. I wonder why this would not throw? It is indeed not written, I don't understand why this does not throw!?

I see this freeze option (great!) - if one of our consumers wishes to set freeze = false and also edit our properties, then they can cast the object as any and then write, e.g. (<any>tx).gasPrice = .... Which seems like an ok-ish solution to me, since you explicitly cast it as any so you can write to it.

[holgerd77]

@jochem-brouwer thanks for giving this some thought! 🙂 Is this any-casting a provable working solution also in a JavaScript context? Or would JavaScript keep the read-only and forget about the any casting (just plainly guessing what could happen)? 🤔

[jochem-brouwer]

@holgerd77 JavaScript doesn't have readonly. Casting it as any in TypeScript makes it "forget" that it is readonly. TypeScript seems to statically check (if you are writing to a property) if it is allowed to do this (if it was strictly readonly at a technical level then casting it as any would also not allow this because it would be technically impossible). In general, think of JavaScript having each type as any 😄 So you can just write to anything you assigned readonly in TypeScript, and you can also call private functions just fine.

If you run your code as above in node, you will see that you can edit it just fine if you pass freeze = false. 👍