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 accountId #152

Merged
merged 1 commit into from
Aug 22, 2024
Merged

feat: add accountId #152

merged 1 commit into from
Aug 22, 2024

Conversation

jaypaik
Copy link
Collaborator

@jaypaik jaypaik commented Aug 22, 2024

Motivation

On-chain identifier for accounts is useful for being able to identify and differentiate between different account implementations easily.

Solution

Implement accountId, inspired by ERC-7579. Note I used a different delimiter /. I think this makes for slightly easier parsing than what the 7579 spec prescribes (they use a . delimiter), since periods always exist in the version number token. I could be convinced to just conform to 7579's spec for alignment.

@jaypaik jaypaik requested a review from a team August 22, 2024 19:07
@jaypaik jaypaik force-pushed the 08-22-feat_add_accountId branch 2 times, most recently from a6375b5 to 8768dc6 Compare August 22, 2024 19:13
@jaypaik jaypaik force-pushed the 08-22-feat_add_accountId branch from 8768dc6 to 9b68e45 Compare August 22, 2024 19:18
Copy link
Contributor

@adamegyed adamegyed 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!

@jaypaik jaypaik merged commit c2072a1 into develop Aug 22, 2024
3 checks passed
@0xrubes
Copy link

0xrubes commented Aug 23, 2024

It seems like in both “.” and “/“ cases, we would have the requirement that the first two occurrences are the actual separators. A slash does seem less likely to be used in tokens than a dot, but I wonder if it is worth it to have this super slight divergence from 7579 🤔

@jaypaik
Copy link
Collaborator Author

jaypaik commented Aug 26, 2024

It seems like in both “.” and “/“ cases, we would have the requirement that the first two occurrences are the actual separators. A slash does seem less likely to be used in tokens than a dot, but I wonder if it is worth it to have this super slight divergence from 7579 🤔

I'm flexible here. I just think . isn't the best choice since it's guaranteed that the semver token would contain it, making it difficult for clients to just do something like accountId.split(".") to get the relevant tokens. But it's still relatively easy (just split on the first 2 dots, ignore the rest), as long as we enforce that the first 2 tokens don't contain the delimiter.

@jaypaik
Copy link
Collaborator Author

jaypaik commented Aug 26, 2024

Aligning with 7579 sounds good to me. Will submit a follow-up PR. @kopy-kat @leekt heads up!

@jaypaik jaypaik deleted the 08-22-feat_add_accountId branch October 9, 2024 00:02
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.

3 participants