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

feat: add starknet.id getters #400

Merged
merged 8 commits into from
Dec 2, 2022

Conversation

fricoben
Copy link
Contributor

close #382

I needed to use some of our code as utils (because of our encoding algorithm), so I created my own utils file (these functions will only be used for starknet id so it makes sense to create our own file).

I tried to be as clean as possible and everything works smoothly. Tell me what you think.

@netlify
Copy link

netlify bot commented Nov 17, 2022

Deploy Preview for starknetjs canceled.

Name Link
🔨 Latest commit 81f5d71
🔍 Latest deploy log https://app.netlify.com/sites/starknetjs/deploys/6389b40e220f9000083121e1

src/account/default.ts Outdated Show resolved Hide resolved
src/account/default.ts Outdated Show resolved Hide resolved
src/account/default.ts Outdated Show resolved Hide resolved
src/account/interface.ts Outdated Show resolved Hide resolved
src/account/interface.ts Outdated Show resolved Hide resolved
@ivpavici
Copy link
Collaborator

ivpavici commented Nov 17, 2022

@0xBenaparte left a couple of cosmetic comments, plus please add the changes in documentation! :)

Other than those I have nothing else, maybe someone else can comment the fact that the logic is inside the Account @tabaktoni @dhruvkelawala

@dhruvkelawala
Copy link
Collaborator

I don't think so starknet.id should be a part of starknet.js. I think it's a better idea to create a separate lib for all the encoding and decoding.

@ivpavici
Copy link
Collaborator

@janek26
Copy link
Member

janek26 commented Nov 21, 2022

I'm with Dhruv on this one. I dont think it should be part of starknet.js for now.
We dont know how/if ENS will enter Starknet etc.

In any case I would avoid to change the Provider/Account interface

@ivpavici
Copy link
Collaborator

What do you think about some compromise, like Ethers has a separate EnsResolver ? https://docs.ethers.io/v5/api/providers/provider/#EnsResolver

Btw I'm sorry this discussion is happening now when the work by @0xBenaparte is already done

@fricoben
Copy link
Contributor Author

I think it would be easier for all starknet developers if it is (Loaf from Realms, @fracek, and Astraly directly asked us to do it).

If ENS enter starknet (starknet.id gonna make a bridge in the future for it) there is still no problem cause you can access them through another hook (getEnsName !== getStarkName).

To avoid confusion, I could not place it in the account class but more as a util that can be accessed through starknet.js (cause at the end of the day considering the popularity of starknet.id right now, it'll be util for a lot of devs).

Tell me what you think.

@fricoben
Copy link
Contributor Author

fricoben commented Nov 25, 2022

Hey @janek26,

As discussed with @ivpavici I removed my changes to the account interface. Tell me if it's ok for you.

Copy link
Member

@janek26 janek26 left a comment

Choose a reason for hiding this comment

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

Yep, much happier! Thanks

src/utils/starknetId.ts Outdated Show resolved Hide resolved
src/utils/starknetId.ts Outdated Show resolved Hide resolved
src/utils/starknetId.ts Show resolved Hide resolved
src/utils/starknetId.ts Show resolved Hide resolved
src/utils/starknetId.ts Show resolved Hide resolved
@fricoben
Copy link
Contributor Author

fricoben commented Nov 29, 2022

It is going to be used to allow the usage of non Latin characters but the alphabet extension has not yet be decided.

More info about encoding here ==> https://docs.starknet.id/for-devs/encoding-algorithm

@fricoben
Copy link
Contributor Author

fricoben commented Dec 1, 2022

I made all the changes, tell me what you think, @janek26.

Thanks a lot !

Copy link
Member

@janek26 janek26 left a comment

Choose a reason for hiding this comment

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

looks good! one minor thing which should be done before merge

src/utils/starknetId.ts Outdated Show resolved Hide resolved
@ivpavici ivpavici merged commit d87bc20 into starknet-io:develop Dec 2, 2022
@github-actions
Copy link

github-actions bot commented Dec 5, 2022

🎉 This PR is included in version 4.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a starknet id hook to starknet js
4 participants