fix(btc): fix jazzicons generations #27662
Conversation
|
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. |
Builds ready [b3563ec]
Page Load Metrics (1827 ± 69 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [bef6290]
Page Load Metrics (2035 ± 89 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ccharly
force-pushed
the
fix/btc-jazzicons
branch
from
bef6290
to
239ca2f
Compare
Builds ready [239ca2f]
Page Load Metrics (1973 ± 72 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ccharly
force-pushed
the
fix/btc-jazzicons
branch
2 times, most recently
from
1a982ee
to
5cd471e
Compare
ccharly
force-pushed
the
fix/btc-jazzicons
branch
from
5cd471e
to
acf55ff
Compare
Builds ready [acf55ff]
Page Load Metrics (1942 ± 128 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| @@ -14,6 +18,21 @@ import { | |||
| AvatarAccountProps, | |||
| } from './avatar-account.types'; | |||
|
|
|||
| // TODO: This might not scale well with our new multichain initiative since it would require | |||
There was a problem hiding this comment.
you can make it a little more ready for extensibility with an approach like that. wdyt?
type ChainType = 'btc' | 'eth';
function getChainType(address: string): ChainType | undefined {
if (isBtcMainnetAddress(address) || isBtcTestnetAddress(address)) {
return 'btc';
}
// Default to 'eth' for other cases, can be extended as needed
return 'eth';
}
function getJazziconNamespace(address: string): string | undefined {
const chainType = getChainType(address);
switch (chainType) {
case 'btc':
return 'bip122';
case 'eth':
return undefined; // Falls back to default Jazzicon behavior
default:
return undefined;
}
}
There was a problem hiding this comment.
I like this idea, lemme address this now!
…azzicon namespace logic
ccharly
force-pushed
the
fix/btc-jazzicons
branch
from
52aae8f
to
4622980
Compare
Builds ready [4622980]
Page Load Metrics (1714 ± 90 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
shared/lib/multichain.ts
Outdated
| /** | ||
| * Multi-chain family type. | ||
| */ | ||
| export enum MultichainType { |
There was a problem hiding this comment.
I'm not sure if MultichainType is the right name here, something like Namespace or Caip2Namespace would make more sense. However, we should pay attention to not reuse a type already used in @metamask/utils.
There was a problem hiding this comment.
I initially wanted to use the one coming from @metamask/utils but we have no Bip122 yet 😅
I can try to update this too, I started a PR some days ago here:
There was a problem hiding this comment.
I'm now using KnownCaipNamespace for this type instead!
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@metamask/utils@9.2.1 |
ccharly
force-pushed
the
fix/btc-jazzicons
branch
from
a266559
to
3cb0db7
Compare
…ddress -> getCaipNamespaceFromAddress
ccharly
force-pushed
the
fix/btc-jazzicons
branch
from
3cb0db7
to
65831ce
Compare
There was a problem hiding this comment.
Hey @ccharly, this is working great! I had one question about the logic for the getCaipNamespaceFromAddress but otherwise LGTM!
- Pulled branch and checked locally ✅
- Ensured jazzicons remain the same after hard reset ✅
Also are we aware that the Bitcoin account creation modals aren't the same as the ethereum ones? Seems like a bug
bitcoin.accounts.mov
| import { KnownCaipNamespace } from '@metamask/utils'; | ||
|
|
||
| function getJazziconNamespace(address: string): string | undefined { | ||
| const namespace = getCaipNamespaceFromAddress(address); | ||
|
|
||
| switch (namespace) { | ||
| case KnownCaipNamespace.Bip122: | ||
| return namespace; | ||
| case KnownCaipNamespace.Eip155: | ||
| return undefined; // Falls back to default Jazzicon behavior | ||
| default: | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Question: Why is getJazziconNamespace defined in the AvatarAccount component instead of within the Jazzicon component itself?
This function appears to be specific to Jazzicon behavior and isn't used for Blockies. It seems like it might be more appropriate to encapsulate this logic within the Jazzicon component for better separation of concerns and to keep the AvatarAccount component more generic?
There was a problem hiding this comment.
Actually, you are 100% right, this makes more sense to move it there. I'm changing that!
So yes, they look different indeed and that's not a bug! 😄 The Bitcoin accounts are backed by a Snap, and we recently introduced a new modal when creating Snap accounts and this one is in full screen (which seems to be a restriction for now when using the confirmation system). We would like to have a very similar modal in the future yes, but for now, we live with this one! |
|
|
Thanks for the clarification @ccharly that makes sense! |
Builds ready [0d565f3]
Page Load Metrics (1843 ± 100 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
the
release-12.7.0
gauthierpetetin
added
release-12.6.0
Description
The jazzicons were all the same for mainnet/testnet accounts. It was probably due to the fact that the namespace being used was
eip155for all addresses, but Bitcoin addresses have a different format.Here's the technical details:
namespacealways defaults toeip155jsNumberForAddresswhich is ethereum-specific (or more like, "hex-specific" here):0xprefix) +parseInt(addr, 16)will only work hex-strings, but Bitcoin is not using this formatparseInthere will only consider the first valid hex-characters of its inputTo fix this, we check for the current address used for the jazzicon and change the namespace based on this.
Ideally, we would want to use the
InternalAccountobject directly, but that would require quite a lot of changes, so for now we keep this simple.Related issues
N/A
Manual testing steps
yarn start:flaskScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist