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.

Sign up for GitHub

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

Jump to bottom

Util: Static Constructors for Withdrawals & Requests (Tree Shaking) #3589

Merged
merged 16 commits into from
Aug 29, 2024
Merged

Util: Static Constructors for Withdrawals & Requests (Tree Shaking) #3589

merged 16 commits into from
Aug 29, 2024

Conversation

ScottyPoi
Copy link
Contributor

@ScottyPoi ScottyPoi commented Aug 14, 2024 •
edited
Loading

Util treeshaking task cleanup from #3560

  • withdrawal.ts
    • Withdrawal static constructors extracted to functions
  • requests.ts renamed request.ts
    • static constructors extracted to functions
      • WithdrawalRequest
      • ConsolidationRequest
      • DepositRequest

@holgerd77
Copy link
Member

In-between feedback: this looks good so far

@ScottyPoi
Copy link
Contributor Author

In-between feedback: this looks good so far

Ran out of time before the weekend. Will finish this up today

@codecov Codecov
Copy link

codecov bot commented Aug 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 71.05263% with 33 lines in your changes missing coverage. Please review.

Project coverage is 20.69%. Comparing base (4470cc3) to head (2c368ea).
Report is 21 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain 84.92% <ø> (?)
client ?
common 89.68% <ø> (?)
devp2p 0.00% <ø> (?)
genesis 0.00% <ø> (?)
tx ?
util 71.45% <71.05%> (?)
wallet ?

Flags with carried forward coverage won't be shown. Click here to find out more.

scorbajio
scorbajio reviewed Aug 23, 2024
View reviewed changes
packages/util/src/request.ts Outdated
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/no-use-before-define */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could disabling no-use-before-define be avoided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

packages/util/src/withdrawal.ts Outdated
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/no-use-before-define */
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here, could disabling no-use-before-define be avoided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

holgerd77
holgerd77 previously requested changes Aug 26, 2024
View reviewed changes
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.

Some basic structural things to discuss respectively adjust.

packages/block/test/eip4895block.spec.ts Outdated Show resolved Hide resolved
packages/util/src/request.ts Show resolved Hide resolved
@scorbajio scorbajio mentioned this pull request Aug 26, 2024
Merged
@scorbajio
Copy link
Contributor

Please update the example embeddings in the util docs and any other package docs that have modified examples from this PR. Also see here for more details.

@holgerd77
Copy link
Member

Can this be finalized so that we can merge?

@ScottyPoi
Copy link
Contributor Author

changes addressed. ready for re-review

acolytec3
acolytec3 approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@acolytec3 acolytec3 dismissed holgerd77’s stale review August 29, 2024 18:59

Has been addressed

@acolytec3 acolytec3 merged commit dc906d0 into master Aug 29, 2024
38 checks passed
@scorbajio scorbajio deleted the util-treeshake branch August 30, 2024 17:15
@holgerd77
Copy link
Member

Nice, looks good! 🙂

@holgerd77 holgerd77 changed the title Util: treeshaking tasks Util: Static Constructors for Withdrawals & Requests (Tree Shaking) Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scorbajio scorbajio scorbajio left review comments

@acolytec3 acolytec3 acolytec3 approved these changes

@holgerd77 holgerd77 holgerd77 left review comments

Assignees
No one assigned
Labels
breaking package: util PR state: needs review target: master Work to be done towards master branch type: refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ScottyPoi @holgerd77 @scorbajio @acolytec3