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.

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

Feat/add app metadata controller #5325

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

frankvonhoven
Copy link

@frankvonhoven frankvonhoven commented Feb 13, 2025

Explanation

Porting over the AppMetadataController from Extension

References

Changelog

@metamask/package-a

  • : Your change here
  • : Your change here

@metamask/package-b

  • : Your change here
  • : Your change here

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
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

I see that this controller has no public method, how do we intend the client to interact with this package?
I just realized that the extension is already using the package via the constructor alone, which I feel is a little weird, but I guess goes beyond the scope of this PR

@@ -0,0 +1,73 @@
{
"name": "@metamask/app-metadata-controller",
"version": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Can we set the version to 0.0.0 until the first release?

Suggested change
"version": "1.0.0",
"version": "0.0.0",

Comment on lines +50 to +53
"@metamask/base-controller": "^8.0.0",
"@metamask/rpc-errors": "^7.0.2",
"@metamask/utils": "^11.1.0",
"nanoid": "^3.3.8"
Copy link
Member

Choose a reason for hiding this comment

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

Are these dependencies used?

Suggested change
"@metamask/base-controller": "^8.0.0",
"@metamask/rpc-errors": "^7.0.2",
"@metamask/utils": "^11.1.0",
"nanoid": "^3.3.8"
"@metamask/base-controller": "^8.0.0"

Comment on lines +153 to +157
/**
* Updates the currentAppVersion in state, and sets the previousAppVersion to the old currentAppVersion.
*
* @param maybeNewAppVersion
*/
Copy link
Member

Choose a reason for hiding this comment

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

nit: Perhaps we can specify what maybe means in the jsdoc:

Suggested change
/**
* Updates the currentAppVersion in state, and sets the previousAppVersion to the old currentAppVersion.
*
* @param maybeNewAppVersion
*/
/**
* Update `currentAppVersion` in the controller state if the provided value
* is different from the current one. When the value is updated, the previous
* value is stored in `previousAppVersion`.
*
* @param maybeNewAppVersion - The new app version to set.
*/

Copy link
Member

Choose a reason for hiding this comment

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

I see that these were simply moved from the extension, but this suggestion will fix the failing CI (or at least part of it)

Comment on lines +169 to +173
/**
* Updates the migrationVersion in state.
*
* @param maybeNewMigrationVersion
*/
Copy link
Member

Choose a reason for hiding this comment

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

nit: similarly to the other function:

Suggested change
/**
* Updates the migrationVersion in state.
*
* @param maybeNewMigrationVersion
*/
/**
* Update `currentMigrationVersion` in the controller state if the provided value
* is different from the current one. When the value is updated, the previous
* value is stored in `previousMigrationVersion`.
*
* @param maybeNewMigrationVersion - The new migration version to set.
*/

@mikesposito
Copy link
Member

mikesposito commented Feb 20, 2025

Could we add a root src/index.ts that exports the relevant objects?
i.e.

// src/index.ts
export * from './AppMetadataController`

@mikesposito
Copy link
Member

mikesposito commented Feb 20, 2025

We should also add these changes to configuration files:

  • .github/CODEOWNERS for assigning the relevant owners to this package
  • tsconfig.json and tsconfig.build.json for adding the package to the workspace

Also, README should also be updated by running yarn update-readme-content

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants