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

util: Replace account static constructors #3524

Merged
merged 6 commits into from
Jul 23, 2024
Merged

util: Replace account static constructors #3524

merged 6 commits into from
Jul 23, 2024

Conversation

ScottyPoi
Copy link
Contributor

@ScottyPoi ScottyPoi commented Jul 19, 2024

continues with #3487

Replaces static constructors for Account class with functions exported from account.ts

  • Account.fromRlpSerializedAccount(...) > accountFromRlp(...)
  • Account.fromRlpSerializedPartialAccount(...) > accountFromPartialRlp(...)
  • Account.fromValuesArray(...) > accountFromValuesArray(...)
  • Account.fromAccountData(...) > accountFromAccountData(...)
  • Account.fromPartialAccountData(...) > accountFromPartialAccountData(...)

@ScottyPoi ScottyPoi changed the title Account static constructors Replace account static constructors Jul 19, 2024
@ScottyPoi ScottyPoi changed the title Replace account static constructors util: Replace account static constructors Jul 19, 2024
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Please stay in the nameing scheme we started with, so createAccountFromAccountData().

Possible Monorepo Wide Naming Simplifications

We can discuss about nameing simplifications (also just realizing that the above one actually is pretty long). But if we do we should do it consistently accross all libraries.

I would e.g. first round suggest that we generally switch over for the "base" (so: the *from*Data() constructors to a simplified naming scheme and drop the from*Data() part, so .e.g.:

  • createAccountFromAccountData() -> createAccount()
  • create1559FeeMarketTxFromTxData() -> create1559FeeMarketTx()
    (here I would value some additional opinion also from others if we would want to drop or keep the FeeMarket naming addition, the names get somewhat long, on the other hand this helps on expressiveness and code readability. I have some slight tendency to keep, but would have me overvoted)
  • createBlockFromBlockData() -> createBlock()

I think these more basic names are still "expressive" enough since this (passing in separate values/parameters) is just the natural thing to do/expect.

RLP Name

  • Account.fromRlpSerializedAccount -> createAccountFromRlp

I do like this naming simplification, think still expressive enough. If we do we should also do monorepo wide.

@ScottyPoi I would suggest that you finish this PR with the old naming conventions and then we eventual monorepo-wide name changes on the stuff we discussed in a separate follow-up PR.

@holgerd77
Copy link
Member

Ok, I talked this through with Jochem, not sure if you have seen the #work channel discussion, and we have some agreement on the renamings. 🙂

So to summarize here (so this goes monorepo-wide then):

  1. We rename/shorten the from[Library]Data names, example: createAccountFromAccountData() -> createAccount()
  2. We shorten the fromRlpSerialized[Library] names, let's please also do: Rlp -> RLP as we have in Block already, example: createAccountFromRlpSerializedAccount -> createAccountFromRLP
  3. We rename the fromValuesArray names to a more expressive fromBytesArray, example : createAccountFromValuesArray -> createAccountFromBytesArray

If you go along, from my side you/we can then also directly do for Account within this PR. We should just remember to then also do a follow-up PR and do it monorepo-wide.

@jochem-brouwer
Copy link
Member

I have done this for the partial accounts: createPartialAccount and createPartialAccountFromRLP. Not entirely sure if this naming is intuitive/correct though. It was accountFromPartialAccountData before I changed it.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @ScottyPoi and @jochem-brouwer! 🙂

@holgerd77 holgerd77 merged commit 0e18cb2 into master Jul 23, 2024
34 checks passed
@holgerd77 holgerd77 deleted the account-static branch July 23, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants