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

fix: Resolve errors, omissions, duplicates in Engine types Global{Actions,Events} #12407

Merged

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Nov 24, 2024

Description

The GlobalActions and GlobalEvents types have not been updated to keep up with the controller V2 upgrades. This commit adds missing entries and fixes redundant, outdated, or erroneous entries.

All changes in this commit are purely at the type-level with no impact on runtime behavior.

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@MajorLift MajorLift self-assigned this Nov 24, 2024
@MajorLift MajorLift added team-wallet-framework team-tiger Tiger team (for tech debt reduction + performance improvements) labels Nov 24, 2024
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@MajorLift MajorLift added the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 24, 2024
Copy link
Contributor

github-actions bot commented Nov 24, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 5b79f71
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/cdab6387-eff2-4a65-83d7-9d6ee48697e8

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@MajorLift MajorLift added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 24, 2024
Copy link
Contributor

github-actions bot commented Nov 24, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 508dc9a
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4c3a06f1-5297-42d3-99c8-414200b9f814

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@MajorLift MajorLift changed the title fix(engine): Fix types GlobalActions, GlobalEvents by adding missing entries and removing redundant, outdated entries fix: Engine types Global{Actions,Events} Nov 24, 2024
@MajorLift MajorLift marked this pull request as ready for review November 24, 2024 13:16
@MajorLift MajorLift requested a review from a team as a code owner November 24, 2024 13:16
MajorLift

This comment was marked as outdated.

@MajorLift MajorLift added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 25, 2024
Copy link
Contributor

github-actions bot commented Nov 25, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: f18f117
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/59052ee7-6a1f-4761-ac56-8701882f433d

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@MajorLift MajorLift changed the title fix: Engine types Global{Actions,Events} fix: Resolve errors, omissions, duplicates in Engine types Global{Actions,Events} Nov 25, 2024
Copy link
Contributor Author

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

Fixes errors with notifications controllers action types.

@mcmire
Copy link
Contributor

mcmire commented Nov 27, 2024

@MajorLift How did we know that these types were incorrect before and how do we know that they are correct now? Or, better, is there any way to guarantee that this won't happen again?

@MajorLift MajorLift requested a review from a team as a code owner November 27, 2024 17:23
@MajorLift MajorLift force-pushed the jongsun/fix/Engine/241125-update-global-actions,events branch from 1a957fb to 5aa9e00 Compare November 27, 2024 17:27
@MajorLift MajorLift removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 27, 2024
@MajorLift MajorLift added the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 27, 2024
Copy link
Contributor

github-actions bot commented Nov 27, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: e31fa74
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/0722b2bd-3b5d-43d0-b90e-1735b36b05f5

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 27, 2024

@mcmire The only place the Global{Actions,Events} types are used is in the ControllerMessenger type definition.

And these are the expectations for the Actions, Events generic type parameters of ControllerMessenger:

  • The internal actions/events of all controllers in the engine context are available to the engine's messenger.
  • Conversely, the engine's context contains all controllers required to implement the allowed actions/events specified by the controllers in the engine context.

So in this PR I've simply updated the Global{Actions,Events} types to include all internal actions/events of every V2 controller in the engine context (which is now all of them).

As to how we can keep these up-to-date in the future:

If the GlobalActions or GlobalEvents type is incomplete, or if an invalid entry is added to a restricted messenger's allowlist, we get the following error message:

Type '"ExampleController:exampleAction"' is not assignable to type '"AddressBookController:getState" | "ApprovalController:getState" | "ApprovalController:clearRequests" | "ApprovalController:addRequest" | "ApprovalController:hasRequest" | ... 132 more ... | "AssetsContractController:getERC1155TokenURI"'. Did you mean '"ExampleController:otherAction"'?ts(2820)

A simpler sanity test is instantiating the engine's messenger as ControllerMessenger<never, never>. We get Type 'string' is not assignable to type 'never'.ts(2322) errors in the actions/events allowlist of every restricted messenger instantiation in the Engine class.

@mcmire
Copy link
Contributor

mcmire commented Nov 27, 2024

If the GlobalActions or GlobalEvents type is incomplete, or if an invalid entry is added to a restricted messenger's allowlist, we get the following error message:

This makes sense! I guess I am curious why the global actions/events list fell so far out of sync though. Are we getting type errors on main now which are being somehow suppressed? I worry that it will fall out of sync again in the future and we or someone will have to spend time manually going through and cross-checking them.

@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 27, 2024

Global{Actions,Events} have probably been updated to resolve any errors in main. But the ComposableController update branch #10441 showed a lot of errors, because the stateChange event for all controllers needs to be included to its events allowlist, but many corresponding ControllerEvents types were missing from GlobalEvents.

I think that with the updates in this PR, the only scenario where this becomes relevant in the future is when adding new controllers to engine context. Whoever's doing that will need to know to update Global{Actions,Events} and the allowedEvents constructor option of ComposableController (EngineService will no longer need to be updated manually with this change).

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.

LGTM.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Lgtm

@MajorLift MajorLift added this pull request to the merge queue Dec 1, 2024
Merged via the queue into main with commit f0f851f Dec 1, 2024
43 checks passed
@MajorLift MajorLift deleted the jongsun/fix/Engine/241125-update-global-actions,events branch December 1, 2024 08:53
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2024
@metamaskbot metamaskbot added the release-7.38.0 Issue or pull request that will be included in release 7.38.0 label Dec 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.38.0 Issue or pull request that will be included in release 7.38.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-tiger Tiger team (for tech debt reduction + performance improvements) team-wallet-framework type-tech-debt Technical debt
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants