Skip to content

[Low priority] Fix a bunch of nits in sdk#2418

Merged
mergify[bot] merged 4 commits intodevelopfrom
will-nits
Apr 5, 2022
Merged

[Low priority] Fix a bunch of nits in sdk#2418
mergify[bot] merged 4 commits intodevelopfrom
will-nits

Conversation

@roninjin10
Copy link
Contributor

I explored the sdk via reading all the code and fixing nits as I saw them. I didn't see very many. The code did a really good job of not trying to be clever and just being very straight forward and simple.

  1. When constant strings have an as const it shows the actual constant value as the type when you hover which I always find nice.
  2. Change a function that throws to never type. In typescript never means the function never returns so typescript will still match the old type in a type match.
  3. Remove instances of any. There weren't very many.
  4. Add an isL2Provider type guard since it seems generally useful. If the function returns true typescript will coerce the type into an L2Provider
  5. Add _isL2Provider to interface. If there is an issue with it not being purely private I can change it back but it seems benign.
  6. Make a chain enum because knowing me personally, the odds of me messing this up if I touch this code base using the hardcoded numbers is very high.
  7. Make the type for Omit a little more accurate.

Description
A clear and concise description of the features you're adding in this pull request.

Additional context
Add any other context about the problem you're solving.

Metadata

  • Fixes #[Link to Issue]

@changeset-bot
Copy link

changeset-bot bot commented Apr 5, 2022

🦋 Changeset detected

Latest commit: 2570e1a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@eth-optimism/sdk Minor
@eth-optimism/message-relayer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mergify mergify bot requested review from cfromknecht and tuxcanfly April 5, 2022 09:12
@roninjin10 roninjin10 changed the title [Low priority] Fix a bunch of nits [Low priority] Fix a bunch of nits in sdk Apr 5, 2022
@github-actions github-actions bot added the A-pkg-sdk Area: packages/sdk label Apr 5, 2022
Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

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

Looks good! One nit, then can approve

@mslipper
Copy link
Contributor

mslipper commented Apr 5, 2022

Needs a changeset, then LGTM.

Will Cory added 3 commits April 5, 2022 14:00
chain id enum nit

more utils nits

fix the rest of the nits

fix comment being moved

remove comment that is no longer needed
@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot merged commit a9f8e57 into develop Apr 5, 2022
@mergify mergify bot deleted the will-nits branch April 5, 2022 22:48
nebojsa94 pushed a commit to Tenderly/optimism that referenced this pull request Apr 26, 2022
* Fix a bunch of nits

chain id enum nit

more utils nits

fix the rest of the nits

fix comment being moved

remove comment that is no longer needed

* add comment to isL2Provider so jsdocs works and change comment

* yarn changeset

Co-authored-by: Will Cory <willcory@Wills-MacBook-Pro.local>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
nitaliano pushed a commit that referenced this pull request May 20, 2024
* Fix a bunch of nits

chain id enum nit

more utils nits

fix the rest of the nits

fix comment being moved

remove comment that is no longer needed

* add comment to isL2Provider so jsdocs works and change comment

* yarn changeset

Co-authored-by: Will Cory <willcory@Wills-MacBook-Pro.local>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
theochap pushed a commit that referenced this pull request Dec 10, 2025
Fixes #2418

---------

Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
Co-authored-by: Arun Dhyani <dhyaniarun7@gmail.com>
theochap pushed a commit that referenced this pull request Jan 14, 2026
Fixes #2418

---------

Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
Co-authored-by: Arun Dhyani <dhyaniarun7@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-pkg-sdk Area: packages/sdk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants