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

refactor: split account types #293

Merged
merged 14 commits into from
Apr 25, 2024
Merged

refactor: split account types #293

merged 14 commits into from
Apr 25, 2024

Conversation

ccharly
Copy link
Contributor

@ccharly ccharly commented Apr 23, 2024

This splits the existing KeyringAccount type into two sub-types. Allowing to have "concrete" types for EOA and 4337 accounts.

This is not a breaking change since KeyringAccount now uses a union to match any of the 2 account types.

Relates https://github.com/MetaMask/accounts-planning/issues/385

@ccharly ccharly requested a review from a team as a code owner April 23, 2024 08:16
@ccharly ccharly force-pushed the refactor/split-account-type branch from f85ce44 to faa242d Compare April 23, 2024 10:51
@ccharly ccharly force-pushed the refactor/split-account-type branch from faa242d to bb84603 Compare April 23, 2024 13:18
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

Left some comments about the acronyms capitalization, this is a minor comment that can lead to a breaking change so feel free to ignore them if it's not worth it

src/base-types.ts Outdated Show resolved Hide resolved
src/eth/types.ts Show resolved Hide resolved
src/eth/types.test-d.ts Show resolved Hide resolved
src/eth/types.ts Show resolved Hide resolved
src/eth/types.ts Show resolved Hide resolved
@gantunesr gantunesr added the team-accounts This should be handled by the Accounts Team label Apr 23, 2024
@gantunesr
Copy link
Member

is there a ticket we can link this PR to?

@ccharly
Copy link
Contributor Author

ccharly commented Apr 24, 2024

The only issue I can think of right now would be this one: https://github.com/MetaMask/accounts-planning/issues/385

gantunesr
gantunesr previously approved these changes Apr 24, 2024
src/api.ts Outdated Show resolved Hide resolved
src/api.ts Outdated Show resolved Hide resolved
src/api.ts Outdated Show resolved Hide resolved
src/api.ts Outdated Show resolved Hide resolved
src/api.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@danroc danroc left a comment

Choose a reason for hiding this comment

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

LGTM

@ccharly ccharly added this pull request to the merge queue Apr 25, 2024
Merged via the queue into main with commit 42752f0 Apr 25, 2024
16 checks passed
@ccharly ccharly deleted the refactor/split-account-type branch April 25, 2024 12:35
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.

3 participants