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

refactor(controller-utils): replace any with types for type safety #3975

Merged
merged 8 commits into from
Mar 5, 2024
Merged

refactor(controller-utils): replace any with types for type safety #3975

merged 8 commits into from
Mar 5, 2024

Conversation

cryptodev-2s
Copy link
Contributor

@cryptodev-2s cryptodev-2s commented Feb 26, 2024 •
edited
Loading

Explanation

The current implementation uses any type for parameters in certain utility functions within controller-utils, which lacks type safety and can lead to runtime errors if incorrect values are passed. This approach needs to change to ensure the reliability and maintainability of the codebase.

References

Changelog

@metamask/controller-utils

Fixed

  • BREAKING: Replaced any type with BN for BNToHex and fractionBN functions to enhance type safety.
  • BREAKING: Replaced any type with unknown in logOrRethrowError.
  • BREAKING: Replaced any type with string in isNetworkType.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@cryptodev-2s cryptodev-2s requested a review from a team as a code owner February 26, 2024 12:24
@cryptodev-2s cryptodev-2s marked this pull request as draft February 26, 2024 12:35
@cryptodev-2s
Copy link
Contributor Author

cryptodev-2s commented Feb 26, 2024 •
edited
Loading

Pipeline is failing for changes made related to forcing BN type. due to a missed up usage on AssetsController which is currently being addressed by #3933

@cryptodev-2s cryptodev-2s force-pushed the refactor/controller-utils-remove-any branch from 328a61d to d508dd1 Compare February 26, 2024 13:46
@cryptodev-2s cryptodev-2s changed the title refactor(controller-utils): replace with types for type safety refactor(controller-utils): replace any with types for type safety Feb 26, 2024
@cryptodev-2s cryptodev-2s force-pushed the refactor/controller-utils-remove-any branch from 181cd7e to d2e119f Compare February 26, 2024 14:07
@cryptodev-2s cryptodev-2s marked this pull request as ready for review February 26, 2024 14:12
@cryptodev-2s cryptodev-2s reopened this Mar 4, 2024
@cryptodev-2s cryptodev-2s mentioned this pull request Mar 4, 2024
Closed
3 tasks
mcmire
mcmire previously approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

A couple of nits, but looks good.

The change to isNetworkType, BNToHex, and fractionBN are breaking — can you note that in the PR description?

packages/controller-utils/src/util.ts Outdated Show resolved Hide resolved
packages/controller-utils/src/util.ts Outdated Show resolved Hide resolved
@cryptodev-2s cryptodev-2s requested a review from mcmire March 5, 2024 10:05
kanthesha
kanthesha approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@kanthesha kanthesha left a comment

Choose a reason for hiding this comment

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

LGTM.

@cryptodev-2s cryptodev-2s merged commit e387b17 into main Mar 5, 2024
139 checks passed
@cryptodev-2s cryptodev-2s deleted the refactor/controller-utils-remove-any branch March 5, 2024 13:52
Gudahtt added a commit that referenced this pull request Mar 5, 2024
@Gudahtt
Merge remote-tracking branch 'origin/main' into batch-queued-requests
ac8b75f 
* origin/main:
  refactor(controller-utils): replace `any` with types for type safety (#3975)
  Release 123.0.0 (#4007)
  [token-detection-controller] Refactor `detectTokens` method (#3938)
  fix: update usage of OP goerli to OP Sepolia (#3999)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikesposito mikesposito mikesposito left review comments

@kanthesha kanthesha kanthesha approved these changes

@MajorLift MajorLift Awaiting requested review from MajorLift

@mcmire mcmire Awaiting requested review from mcmire

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

controller-utils: Replace use of any with proper types (non-test files only)
5 participants
@cryptodev-2s @mcmire @MajorLift @mikesposito @kanthesha