Breaking up the Transaction class

[s1na]

Created this issue to continue the discussion started in ethereumjs/ethereumjs-tx#142 and to garner feedback whether you think this'd make sense. Here's a rough sketch of one (non-breaking) approach that comes to my mind:

Make TxData a class, and use it to store the actual data for the transaction, plus methods for serialization/deserialization. In Transaction's constructor instantiate TxData and store it for later use (possibly add a static fromTxData method). Logic methods stay on Transaction and use TxData as source of data but do not mutate it.

If this is too vague I can try to write up (or a diagram) of something more concrete. Please feel free to propose other ideas.

[alcuadrado]

I see the issues in ethereumjs/ethereumjs-tx#140 and ethereumjs/ethereumjs-tx#150 related to this one, and that's why I proposed moving those conversations here.

IMO the problems we are seeing come from a few design desitions:

1. The internal representation of the transaction values is completely coupled to RLP. This leads to confusing things, like leading 0s being removed unexpectedly.

2. Unsigned transactions have bogus signatures. This is confusing both as a developer of the lib and as a user of it, for example tx.hash(false) is weird.

3. Many operations mutate the internal state and these side effects are used internally in unexpected ways. A clear example of this is Transaction.prototype.getSenderPublicKey's implementation.

Created this issue to continue the discussion started in ethereumjs/ethereumjs-tx#142 and to garner feedback whether you think this'd make sense. Here's a rough sketch of one (non-breaking) approach that comes to my mind:

Make TxData a class, and use it to store the actual data for the transaction, plus methods for serialization/deserialization. In Transaction's constructor instantiate TxData and store it for later use (possibly add a static fromTxData method). Logic methods stay on Transaction and use TxData as source of data but do not mutate it.

If this is too vague I can try to write up (or a diagram) of something more concrete. Please feel free to propose other ideas.

What I don't understand is how to make this in a non-breaking way. At least not without keeping everything with side-effects.

[s1na]

What I don't understand is how to make this in a non-breaking way.

Hm, you're right. It could be possible to have it non-breaking by having getters for values on Transaction. But that won't be an improvement to the current design...

@alcuadrado What do you propose? I think some breaking change is inevitable.

[alcuadrado]

A good way to move forward may be to do a clean redesign of the library, and then evaluate if it makes sense to replace it entirely or only we want part of the changes. There's the possibility of some work not ending up in the library, but we get to experiment with solving all those things.

I can do this if we agree that it's something that we want to do.

@danjm, you mentioned in ethereumjs/ethereumjs-tx#142 that you had some ideas about redesigning this lib. Mind sharing them? Thanks!

[rajeshsubhankar]

[DELETED BY EDITOR @holgerd77, OUT OF TOPIC]

[holgerd77]

Hi @rajeshsubhankar, your comment had absolutely nothing to do with the issue discussed here, please make sure that you do comments or open issues where it fits.

Have deleted your post and re-posted as a new issue, see above.

[alcuadrado]

I agree with moving that to a different issue, but we should note that what @rajeshsubhankar describes as "Use case 2" is important for this discussion.

Basically, he is having problems with setting and verifying a signature after constructing the TX object. The problem is that the TX object only computes the chainId on construction, not on the v setter.

[alcuadrado]

Hey @s1na, I created PR ethereumjs/ethereumjs-tx#154 to explore one possibility of what we can do with this and other libraries.

The idea of that PR is to keep everything as explicit as possible. Transaction APIs now work with buffers, with the only exception of static factory methods added for the users' convenience.

In this new version, transactions aren't immutable either, but only sign and the setters mutate them.

It has about 60 lines more of boilerplate, but I think it's a fair price for making it much easier to understand and work with.

PS: I really dislike some of the names I gave to interfaces.

[ryanio]

Resolved with #812