-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
chore: erc3770 chain specific addresses #3848
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Skipped Deployments
|
Coverage Report
File Coverage
|
…nd remove old implementation
@tomiir I added viem for address validation. I don't think the bundle size is a concern for experimental package. Wdyt |
describe('ConverterUtil', () => { | ||
describe('convertCaip10ToErc3770', () => { | ||
it('should convert Ethereum mainnet CAIP address to ERC-3770 format with checksum', () => { | ||
const caipAddress = 'eip155:1:0xfb6916095ca1df60bb79ce92ce3ea74c37c5d359' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what I call a test file
namespace === undefined || | ||
chainId === undefined || | ||
address === undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we need explicit check agains undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript will complain down below when using the chainId and namespace so I just added validation for everything in one go
} | ||
|
||
const createErc3770Address = (address: string, chainId: string): string => { | ||
const shortName = ConstantsUtil.CHAIN_NAMES[chainId] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this something we need to mantain or is there a public registry we can use? should this maybe be some api endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a public registry https://github.com/ethereum-lists/chains
I would consider moving it to an API or just modifying the json with the chains we support if we wanted to go in prod with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just query them from here (not directly ofc) https://chainid.network/chains_mini.json, but its an additional unnecessary request atm
Description
Adding support for displaying chain-specific addresses following the ERC-3770. When a user connects with an EIP-155 compatible wallet, the address will be displayed in both CAIP and ERC-3770 formats.
We're also exporting a utility from experimental package to convert CAIP-10 to ERC-3770
and another one to construct ERC3770 address on its own
Type of change
Associated Issues
For Linear issues: Closes APKT-xxx
For GH issues: closes #...
Showcase (Optional)
Checklist