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

Remove use of ethereumjs-util defineProperties #62

Closed
wants to merge 2 commits into from

Conversation

@ryanio
Copy link
Contributor

ryanio commented Jan 10, 2020

This PR is a first pass solution to close #29, removing the use of ethereumjs-util's defineProperties for a more explicit type-safe alternative.

The change is a bit verbose, however it does follow the suggestion of #27 (comment) that adding some boilerplate is worth it.

@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Jan 10, 2020

Coverage Status

Coverage increased (+14.4%) to 56.863% when pulling ccef85e on removeDefineProperties into 485cddb on master.

@s1na

This comment has been minimized.

Copy link
Contributor

s1na commented Jan 13, 2020

We avoided making a breaking API change at the same time as the Typescript transition. IMO when removing defineProperties it'd be a good chance to also make the API more strict. I.e. instead of accepting anything and converting it to the form we want, accept the form we want and validate to make sure its right. More concretely:

  • For parameters accept Buffer and BN values
  • Either accept fields as separate arguments to constructor (e.g. constructor (nonce: BN, balance: BN, etc.)) or an object. No need to support lists as well.

This might not be great for devex though. Perhaps @alcuadrado can chime in here.

If we don't want to release a breaking change, then I request a few changes to the PR. As it stands the PR isn't a full replacement for defineProperties. The logic implemented in constructor needs to be performed for every field's setter (i.e. when i say account.nonce = '0x3, it should convert it to buffer etc.). It's also field getters, the toJSON and serialize methods. I'm not sure, but depending libraries might be using the raw field in some way, this should be investigated.

Btw, for some more general discussion on a similar note see: ethereumjs/ethereumjs-tx#142, ethereumjs/ethereumjs-tx#151, and ethereumjs/organization#56

@ryanio

This comment has been minimized.

Copy link
Contributor Author

ryanio commented Jan 13, 2020

Thanks for the context @s1na :) there has been quite a bit of discussion and still trying to get around all of it.

If we do want to go for breaking changes, I suggest we also go for immutability as suggested in problem #3 of ethereumjs/organization#56. This PR was initially meant more of an incremental step to tackle problem #1.

Thanks for the notes about the other functionality of defineProperties. For now I will add the rest of the features without any breaking changes and we can make a decision if we are open to changing the api now, in this PR, or at some point later, such as after monorepo migration, when we can have a more comprehensive cascading test suite that may detect any issues.

@alcuadrado

This comment has been minimized.

Copy link
Member

alcuadrado commented Jan 13, 2020

This might not be great for devex though

Expecting everything in their proper type can be frustrating to users, as everyone uses strings/numbers everywhere now.

I think this can be handled with factory methods if we are consistent with them and document them preferred way of doing things. This gives us the advantage to only deal with the right types inside the class, without disrupting devex too much.

I proposed this in ethereumjs/organization#56

@ryanio

This comment has been minimized.

Copy link
Contributor Author

ryanio commented Jan 13, 2020

This gives us the advantage to only deal with the right types inside the class, without disrupting devex too much.

Yes very much looking forward to this in the future, for example nonce and balance should ideally be internally stored/typed as BN.


I committed some new code that adds getters/setters, raw, toJson. Take a look and let me know. My goal for right now is to not introduce any breaking changes until the monorepo, but instead just use this PR as a small step to removing the ethereumjs-account dependency on defineProperties.

I purposefully left out a setter for codeHash since I assume that cannot change after initial object construction.


Also, is this a bug that toJson(label: true) would return the js obj without JSON.stringifying it first? https://github.com/ethereumjs/ethereumjs-util/blob/master/src/object.ts#L22-L30

@holgerd77

This comment has been minimized.

Copy link
Member

holgerd77 commented Jan 20, 2020

@s1na @alcuadrado Can one of you have another look here?

// and setters to have type `any` to be cast `toBuffer`,
// however get ts error:
// `'get' and 'set' accessor must have the same type`
type BufferLike = Buffer | any

This comment has been minimized.

Copy link
@alcuadrado

alcuadrado Jan 21, 2020

Member

When migrating -tx to TS we decided to just set all properties as Buffer, but keep the toBuffer logic in the setters. The rationale was that this is only a breaking change for TS users, who would appreciate the stricter types. Note that we released it as a major version though.

Also, we have a stricter BufferLike here: https://github.com/ethereumjs/ethereumjs-tx/blob/master/src/types.ts#L19

export default class Account {
private props: AccountProps

This comment has been minimized.

Copy link
@alcuadrado

alcuadrado Jan 21, 2020

Member

I don't get this. Why not just private fields like _nonce, _balance, etc? Not that I'm against it either, just curious.

This comment has been minimized.

Copy link
@ryanio

ryanio Jan 22, 2020

Author Contributor

I liked it more as a pattern, I'm open to either way. I only did it as a workaround for needing to define getters and setters on the properties. I think if we make the object immutable in the next breaking change revision then we can just define them as normal properties (without needing _ or props).

This comment has been minimized.

Copy link
@alcuadrado

alcuadrado Jan 23, 2020

Member

Yeah, that would be great. Just set normal properties in the constructor :)

}

set nonce(nonce: BufferLike) {
this.props.nonce = toBuffer(nonce)

This comment has been minimized.

Copy link
@alcuadrado

alcuadrado Jan 21, 2020

Member

These things always confuse me, but isn't the nonce a number? As per RLP it should have it's leftmost zeros stripped.

This comment has been minimized.

Copy link
@alcuadrado

alcuadrado Jan 21, 2020

Member

On a second thought, account representations are never transmitted, nor used in consensus rules, so I guess we are free to represent them as we want.


/**
* The hash of the code of the contract.
*/
public codeHash!: Buffer
get codeHash(): Buffer {

This comment has been minimized.

Copy link
@alcuadrado

alcuadrado Jan 21, 2020

Member

I get the intention of not including the codeHash setter, and I've been tempted to do things like this a million times while dealing with defineProperties, but I'm not sure if it's worth it, as we don't know much about the use cases of these libraries.

let codeHash

if (typeof data === 'string') {
data = Buffer.from(data.substring(0, 2) === '0x' ? data.substring(2) : data, 'hex')

This comment has been minimized.

Copy link
@alcuadrado

alcuadrado Jan 21, 2020

Member

We normally use toBuffer for this kind of conversions, which has some validation logic. For example, it will throw if a string without the 0x-prefix is passed.

@alcuadrado

This comment has been minimized.

Copy link
Member

alcuadrado commented Jan 21, 2020

Sorry for taking so long to re-review this, @ryanio. Feel free to ping me through gitter if I'm non responsive in a PR/issue.

re toJson(true): It may have been accidental, and it's really weird, but fixing it is a breaking change, which may be ok. But if we only change the behaviour in a single library it would be very frustrating for our users. Just as data point, I used toJson(true) extensively in the past.

@ryanio

This comment has been minimized.

Copy link
Contributor Author

ryanio commented Jan 22, 2020

No worries! Thanks for the review and comments.

I'm temped to close this PR for now and revisit it after monorepo migration. Rather than one step now and another step later with breaking changes that would rewrite much of the same code, I'd prefer to improve it properly once with the changes we've identified in our discourse (correct internal data types on properties, simplified constructor with parameters, immutability).

@alcuadrado

This comment has been minimized.

Copy link
Member

alcuadrado commented Jan 23, 2020

Feel free to close it. Attempting to to remove defineProperties and the discussions it sparkled is already valuable imo.

@ryanio ryanio closed this Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.