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.

Sign up for GitHub

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

Jump to bottom

CAIP 10 support (blockchainAccountId) #205

Merged
merged 6 commits into from
Nov 10, 2021
Merged

CAIP 10 support (blockchainAccountId) #205

merged 6 commits into from
Nov 10, 2021

Conversation

daoauth
Copy link
Contributor

@daoauth daoauth commented Oct 28, 2021

Support CAIP 10

  • bip 122
  • eip 155
  • cosmos (need to change the example of cosmos in CAIP10.)

#204

@daoauth daoauth mentioned this pull request Oct 29, 2021
Open
oed
oed suggested changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@oed oed left a comment

Choose a reason for hiding this comment

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

Please don't add a dependency on crypto-js which is pretty large:
https://bundlephobia.com/package/[email protected]

src/blockchains/bip122.ts Outdated
@@ -0,0 +1,13 @@
import SHA256 from 'crypto-js/sha256'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use @stablelib/sha256 which is already a dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix it

src/blockchains/bip122.ts Outdated
@@ -0,0 +1,13 @@
import SHA256 from 'crypto-js/sha256'
import RIPEMD160 from 'crypto-js/ripemd160'
import enc from 'crypto-js/enc-hex'
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use uint8arrays for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix it

src/blockchains/bip122.ts Outdated
@@ -0,0 +1,13 @@
import SHA256 from 'crypto-js/sha256'
import RIPEMD160 from 'crypto-js/ripemd160'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's use ripemd160 pkg? Then we don't need to pull in the large crypto-js package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove crypto-js package

@mirceanis
Copy link
Member

@oed I was about to make the exact same comments 😄
I spent some time today looking at how to avoid any new dependencies.
Glad we're on the same page.

mirceanis
mirceanis approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@mirceanis mirceanis 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 to me

oed
oed suggested changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@oed oed left a comment

Choose a reason for hiding this comment

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

Would be nice to not use nodejs specific Buffer.

src/blockchains/bip122.ts Outdated
export const publicKeyToAddress = (publicKey: string): string => {
const publicKeyBuffer = Uint8Array.from(Buffer.from(publicKey, 'hex'))
const hash = new RIPEMD160().update(Buffer.from(sha256(publicKeyBuffer))).digest()
const step1 = Buffer.concat([Buffer.from('00', 'hex'), hash])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please not use Buffer it's a nodejs only dependency!

Copy link
Contributor

Choose a reason for hiding this comment

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

Should use Uint8Array instead!

Copy link
Contributor

Choose a reason for hiding this comment

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

The uint8arrays pkg is your friend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buffer error from ripemd160, and ripemd160 also use buffer.

ERROR in ./node_modules/ripemd160/index.js 2:13-37
Module not found: Error: Can't resolve 'buffer' in '/home/runner/work/did-jwt/did-jwt/node_modules/ripemd160'

So use crypto-js/ripemd160 package (or ripemd160.js) only, or looking for other way.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, thoughts @mirceanis ? Could crypto-js/ripemd160 cause issues somewhere bc it's js modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make and add new ripemd160.ts.

Copy link
Member

Choose a reason for hiding this comment

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

@daoauth I've been trying to test your changes in react-native (that's where most issues arise with new dependencies), and there are still issues with regards to the use of Buffer.
I suppose that they can be worked around by some rn-nodeify magic, but I would like to keep that option as a last resort, since it complicates all setups.

@oed's suggestion to use uint8arrays instead of Buffer refers to the complete replacement of the nodejs-specific Buffer API.

That being said, I'm not yet comfortable taking on the maintenance burden of a RIPEMD160 implementation.
Ideally there should be a Uint8Array friendly RIPEMD160 implementation we could use.
I haven't yet had time to try the variant with crypto-js/ripemd160; but I see there were some build issues with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mirceanis How about DataView? Can I use it?

Copy link
Member

Choose a reason for hiding this comment

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

wow, that was fast :)
This new commit seems to work ok in my tests.
I'm still reluctant about taking on the RIPEMD160 implementation, but I'm also not aware of trustworthy alternatives.
@oed any insights appreciated ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

DataView seems like a standard lib: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView

@daoauth daoauth requested a review from mirceanis November 3, 2021 15:01
@mirceanis mirceanis merged commit 73cba89 into decentralized-identity:master Nov 10, 2021
@mirceanis mirceanis mentioned this pull request Nov 10, 2021
Closed
uport-automation-bot pushed a commit that referenced this pull request Nov 10, 2021
@semantic-release-bot
chore(release): 5.11.0 [skip ci]
a2021f4 
# [5.11.0](5.10.0...5.11.0) (2021-11-10)

### Features

* CAIP 10 support for bip122 & cosmos ([#205](#205)) ([73cba89](73cba89))
@uport-automation-bot
Copy link
Collaborator

🎉 This PR is included in version 5.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

daoauth added a commit to daoauth/CAIPs that referenced this pull request Nov 10, 2021
@daoauth
Update caip-10.md
191e21b 
update cosmos address use case 

decentralized-identity/did-jwt#205
@daoauth daoauth mentioned this pull request Nov 10, 2021
Closed
daoauth added a commit to daoauth/security-vocab that referenced this pull request Nov 10, 2021
@daoauth
Update Cosmos address (blockchainAccountId)
8fe98bc 
Change cosmos address scheme of blockchainAccountId 

`cosmos:[prefix]:[cosmos_address_without_prefix]`

decentralized-identity/did-jwt#205
@daoauth daoauth mentioned this pull request Nov 10, 2021
Merged
msporny pushed a commit to w3c-ccg/security-vocab that referenced this pull request Nov 10, 2021
@daoauth @msporny
Update Cosmos address (blockchainAccountId)
1030058 
Change cosmos address scheme of blockchainAccountId 

`cosmos:[prefix]:[cosmos_address_without_prefix]`

decentralized-identity/did-jwt#205
@clehner clehner mentioned this pull request Nov 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oed oed oed requested changes

@mirceanis mirceanis Awaiting requested review from mirceanis

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daoauth @mirceanis @uport-automation-bot @oed