Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

[FIX] Inconsistent data types and validations #228

Merged
merged 12 commits into from
May 24, 2023
Merged

[FIX] Inconsistent data types and validations #228

merged 12 commits into from
May 24, 2023

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented May 12, 2023

Description

The TS refactor and workflow update had issues that prevented the module to be correctly published and consumed by the clients, this PR aims to solve them.

The noticeable issues were,

  1. The method fullUpdate was marked as private when it should be public.
  2. The method normalizeToHex is returning a string to methods that require a Hex.
  3. The data type validation for the parameter data in the #newKeyring was removed because the clients are not sending the correct input. The validation will be re-introduced after the proper updates in the clients.
  4. The tsconfig.build.json file was update in accordance to the module template.
  5. the .d.ts files were misplaced in the wrong directory

Changes

  • CHANGED:
    • The methods that consume the method normalizeToHex have to cast the return type as Hex.
    • The method #newKeyring removed a data validation because of the data param input from the clients.
    • The method fullUpdate was mistakenly flagged as private, this PR introduces it again as public.

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation for new or updated code as appropriate (note: this will usually be JSDoc)
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@gantunesr gantunesr marked this pull request as ready for review May 15, 2023 18:44
@gantunesr gantunesr requested a review from a team as a code owner May 15, 2023 18:44
@legobeat
Copy link
Contributor

legobeat commented May 15, 2023

BREAKING:
  The method fullUpdate was mistakenly flagged as private, this PR introduces it again as public.

nit but I don't think that would be breaking either way? Making it private from public would have been.

@legobeat
Copy link
Contributor

legobeat commented May 15, 2023

@gantunesr How can I replicate the release-blocking issue that this addresses locally in order to review the fix?

@gantunesr gantunesr added the team-accounts This should be handled by the Accounts Team label May 16, 2023
@gantunesr
Copy link
Member Author

@legobeat good point!

How can I replicate the release-blocking issue that this addresses locally in order to review the fix?

I'm not sure about the exact problem that led to the error in NPM publishing. I was reviewing the previous workflow but didn't find any logs pertaining to the issue. Previously, I encountered errors concerning the yarn build command and encountered some typescript problems with the clients. @Gudahtt, do you have any suggestions on how we can ensure that the publish workflow is functioning correctly?

@Gudahtt
Copy link
Member

Gudahtt commented May 16, 2023

Try comparing the module template publishing workflow with the one used here. The problem was that the build step was missing entirely - it didn't malfunction, it was just never included.

I don't think it relates to the changes in this PR though, we can fix the workflows separately.

@gantunesr
Copy link
Member Author

gantunesr commented May 17, 2023

@Gudahtt @legobeat I have opened a new PR (#230) to update the module configuration to match the one from https://github.com/MetaMask/metamask-module-template

legobeat
legobeat previously approved these changes May 19, 2023
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

LGTM

@paulmillr
Copy link

any updates on this one?

@gantunesr
Copy link
Member Author

@paulmillr I'll run a test now and if it succeeds it should be good to go

@paulmillr
Copy link

@gantunesr tests seems to pass

@gantunesr
Copy link
Member Author

@paulmillr I was referring to the unit and E2E tests in the MetaMask client, they're failing due to the error management updates. I'm working on fixing them

@gantunesr gantunesr merged commit 20fa615 into main May 24, 2023
@gantunesr gantunesr deleted the fix/build branch May 24, 2023 21:53
@paulmillr
Copy link

Is timing for v11 release known?

@legobeat
Copy link
Contributor

legobeat commented May 27, 2023

Is timing for v11 release known?

@metamask/eth-keyring-controller v11.0.0 has been released: https://github.com/MetaMask/KeyringController/releases/tag/v11.0.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-accounts This should be handled by the Accounts Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants