Skip to content

Conversation

ScottyPoi
Copy link
Contributor

@ScottyPoi ScottyPoi commented Aug 14, 2024

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

Copy link

codecov bot commented Aug 23, 2024

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.

@@ -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.

✔️

@@ -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.

✔️

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.

@am1r021
Copy link
Contributor

am1r021 commented Aug 26, 2024

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

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
@am1r021 am1r021 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants