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

Add Caip Types #116

Merged
merged 14 commits into from
Aug 1, 2023
Merged

Add Caip Types #116

merged 14 commits into from
Aug 1, 2023

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Jul 12, 2023

@jiexi jiexi requested a review from a team as a code owner July 12, 2023 23:34
src/caip-chaid-id.test-d.ts Outdated Show resolved Hide resolved
src/caip-chain-id.ts Outdated Show resolved Hide resolved
src/caip-chain-id.ts Outdated Show resolved Hide resolved
src/caip-chain-id.ts Outdated Show resolved Hide resolved
@mcmire
Copy link
Contributor

mcmire commented Jul 13, 2023

@jiexi Do you mind renaming the title of this PR so that it will be easier to understand at-a-glance in the commit history later on?

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.

Hey @jiexi, I had some comments below.

I know your PR on the extension side is in flux but I think to understand this PR better it'd be nice to know how you plan on using these functions and tools — could you talk about that a bit?

src/caip-chain-id.ts Outdated Show resolved Hide resolved
src/caip-chain-id.ts Outdated Show resolved Hide resolved
src/caip-chain-id.ts Outdated Show resolved Hide resolved
src/caip-chain-id.ts Outdated Show resolved Hide resolved
@jiexi jiexi changed the title Jl/mmp 901/caip 2 Add CaipChainId Jul 18, 2023
Copy link

@hmalik88 hmalik88 left a comment

Choose a reason for hiding this comment

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

What's the motivation behind re-doing the work that already exists in @metamask/snaps-utils? https://github.com/MetaMask/snaps/blob/main/packages/snaps-utils/src/namespace.ts

@jiexi
Copy link
Contributor Author

jiexi commented Jul 20, 2023

What's the motivation behind re-doing the work that already exists in @metamask/snaps-utils? https://github.com/MetaMask/snaps/blob/main/packages/snaps-utils/src/namespace.ts

Nice, @hmalik88 ! Didn't know that this was already done. Looks wonderful! I feel like the term "Chain ID" is pretty overloaded now. I don't believe Namespace nor Reference are very clear terms either even though they are the terms used in the CAIP specs. What are your thoughts on this? I think renaming the types to include a Caip prefix would help make this less ambiguous. Moving the Caip types from @metamask/snap-utils to @metamask/utils might make more sense as well given this will be more applicable to entirety of metamask as we shift towards multichain

@hmalik88
Copy link

@jiexi You're right, it is an overloaded term. We're releasing the name resolver API soon and I was going to make some type changes. Perhaps you're right that we may want to prefix with Caip, but that also means we're making a distinction internally. In what contexts are we using Chain ID? Seems sensible to move to utils, these types were mostly added when our team was doing keyrings work ~ 10 months ago.

@jiexi
Copy link
Contributor Author

jiexi commented Jul 20, 2023

@hmalik88 I'm currently trying to replace our usage of Eth chain id with CAIP chain id. I'm in the process of replacing chainId (eth, decimal or hex) with caipChainId in all of core and extension, so soon all contexts will include CAIP chain id.

Do you happen to know if there is an estimate for when these changes will be available?

@jiexi jiexi changed the title Add CaipChainId Add Caip Types Jul 26, 2023
src/caip-types.ts Outdated Show resolved Hide resolved
src/caip-types.ts Outdated Show resolved Hide resolved
src/caip-types.ts Outdated Show resolved Hide resolved
@jiexi
Copy link
Contributor Author

jiexi commented Jul 26, 2023

I've scrapped my types and brought in Caip types from @metamask/snaps-utils. The plan is to make this the new home for these types and to update all usage of @snap-utils to caip types to this new one

@jiexi jiexi requested a review from mcmire July 26, 2023 21:43
src/caip-types.ts Outdated Show resolved Hide resolved
src/caip-types.ts Outdated Show resolved Hide resolved
src/caip-types.ts Outdated Show resolved Hide resolved
src/caip-types.ts Outdated Show resolved Hide resolved
src/caip-types.ts Outdated Show resolved Hide resolved
src/caip-types.ts Outdated Show resolved Hide resolved
src/caip-types.ts Outdated Show resolved Hide resolved
@jiexi
Copy link
Contributor Author

jiexi commented Jul 28, 2023

Okay. pretty happy with this now

@jiexi jiexi requested a review from hmalik88 July 28, 2023 18:22
@jiexi
Copy link
Contributor Author

jiexi commented Jul 28, 2023

@metamaskbot publish-preview

@adonesky1
Copy link
Contributor

LGTM, however I defer to @hmalik88 and @mcmire if they have any outstanding concerns

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.

Just had a couple of questions, but this all seems reasonable to me.

// Valid caip strings:

expectAssignable<CaipChainId>('namespace:reference');
expectAssignable<CaipChainId>('namespace:');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that this is non-enforceable via TypeScript, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not. We could try to hack it by doing something like this

type Character = "a" | "b" | "c" | "d" | "e" | "f" | "g" | ...

export type CaipChainId = `${Character}${string}:${Character}${string}`;

But I believe this makes a type with n^2 definitions since they get crossmulitplied

expectAssignable<CaipReference>(`${embeddedString}`);

expectAssignable<CaipAccountId>('namespace:reference:accountAddress');
expectAssignable<CaipAccountId>('namespace:reference:');
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar as above — I'm assuming this is non-enforceable via TypeScript?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

Copy link

@hmalik88 hmalik88 left a comment

Choose a reason for hiding this comment

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

LGTM

@adonesky1 adonesky1 merged commit 2653a1a into main Aug 1, 2023
19 checks passed
@adonesky1 adonesky1 deleted the jl/mmp-901/caip-2 branch August 1, 2023 14:13
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.

4 participants