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 SIP-018 support #1284

Conversation

beguene
Copy link
Contributor

@beguene beguene commented Jun 2, 2022

closes #1283 #1281

Those functions are required to support SIP-018 here, in connect and the web-wallet
The following functions have been added
encodeStructuredData
decodeStructuredDataSignature
signStructuredData

Testing information

I added some unit test and the test vectors specified in SIP-018 covering 100% of the new code paths.

Checklist

  • Code is commented where needed
  • Unit test coverage for new or modified code paths
  • npm run test passes
  • Changelog is updated

cc: @janniks @kyranjamie @MarvinJanssen @aulneau

@vercel
Copy link

vercel bot commented Jun 2, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
stacks-js ✅ Ready (Inspect) Visit Preview Jun 3, 2022 at 10:00AM (UTC)

@janniks
Copy link
Collaborator

janniks commented Jun 2, 2022

thoughts on moving the new files/methods to the @stacks/encryption package? currently, this is where all the additional signing related methods are (which are not necessarily needed for transactions) — this way we could keep the encrpytion dep out of transactions deps.

cc @kyranjamie

in the future (maybe next major release), i'd like to move to a separate package that bundles all signing + hashing (and other crypto stuff, with the exception of encryption). this package would then be used for most things and would become a dep of both tx and encryption.

@beguene
Copy link
Contributor Author

beguene commented Jun 2, 2022

@janniks Yes I initially did that but I needed signMessageHashRsv which is in stacks/transactions, and I did not want to include stacks/transaction as a dependency in stacks/encryption. 😞
Indeed all the signature functions should be out of stacks/transactions. Any thoughts on the best way to organise this ?
cc @kyranjamie

@beguene
Copy link
Contributor Author

beguene commented Jun 2, 2022

I could temporarily move my signStructuredData into the web-wallet and just have the core primitive and use
signMessageHashRsv in the wallet itself, until we have this new split of packages in stacks.js

@beguene
Copy link
Contributor Author

beguene commented Jun 2, 2022

or I can just keep signStructuredData in @stacks/transactions and move everything else in @stacks/encryption

@kyranjamie
Copy link
Contributor

Not sure on best way to organise this, but signing methods in encryption package doesn't really make sense.

Either we have a generic @stacks/crypto package, or we make a new one for general message signing? 🤔

@janniks
Copy link
Collaborator

janniks commented Jun 2, 2022

🙃 semantically signing is somewhat closer to tx, as txs need signing (not structured).

in hindsight, signing can only really live in tx unless we want to split structured-signing from signing (even in the monorepo i don't want the package count to explode). so if structured signing needs clarity values (currently in tx, unless we wanna open the next can of worms), and signing anything else would result in circular deps.

so for now, i think we can leave as is — and i'll try to think of how/if this can be improved

Copy link
Collaborator

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

At some point we might need to think about how this type will be serialized. Currently, devs could probably cast it to a clarity buffer and use the reference clarity verifier

@beguene beguene force-pushed the feat/add-sip-018-sign-strucutured-data/I1283 branch from 1786b9f to 1e87cc5 Compare June 3, 2022 09:40
@codecov-commenter
Copy link

Codecov Report

Merging #1284 (1786b9f) into master (dfa7b9a) will increase coverage by 0.18%.
The diff coverage is 100.00%.

❗ Current head 1786b9f differs from pull request most recent head 1e87cc5. Consider uploading reports for the commit 1e87cc5 to get more accurate results

@@            Coverage Diff             @@
##           master    #1284      +/-   ##
==========================================
+ Coverage   64.78%   64.97%   +0.18%     
==========================================
  Files         125      126       +1     
  Lines        8718     8742      +24     
  Branches     1896     1896              
==========================================
+ Hits         5648     5680      +32     
+ Misses       2818     2811       -7     
+ Partials      252      251       -1     
Impacted Files Coverage Δ
packages/transactions/src/constants.ts 100.00% <100.00%> (ø)
...ckages/transactions/src/structuredDataSignature.ts 100.00% <100.00%> (ø)
packages/transactions/src/clarity/serialize.ts 100.00% <0.00%> (+2.00%) ⬆️
packages/transactions/src/errors.ts 65.71% <0.00%> (+17.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfa7b9a...1e87cc5. Read the comment docs.

@beguene
Copy link
Contributor Author

beguene commented Jun 3, 2022

@janniks I added test.each and made a few more changes

  • I changed the function signature for encodeStructuredData it uses now 'named parameters' to avoid order mistakes
export function encodeStructuredData({
  message,
  domain,
}: {
  message: ClarityValue;
  domain: ClarityValue;
}): Buffer {

cc @aulneau @MarvinJanssen

@beguene beguene marked this pull request as ready for review June 3, 2022 09:56
@beguene beguene requested a review from janniks June 3, 2022 09:56
@beguene beguene force-pushed the feat/add-sip-018-sign-strucutured-data/I1283 branch from 1e87cc5 to 1425a8e Compare June 3, 2022 09:59
Copy link

@MarvinJanssen MarvinJanssen left a comment

Choose a reason for hiding this comment

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

LGTM! Excited to try this out soon.

Copy link
Collaborator

@janniks janniks 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! Should we merge+release?

@beguene
Copy link
Contributor Author

beguene commented Jun 7, 2022

Looks good! Should we merge+release?

Yes ! 🚀

@janniks janniks merged commit a4c0577 into hirosystems:master Jun 7, 2022
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.

Add new methods to support SIP018
5 participants