-
Notifications
You must be signed in to change notification settings - Fork 316
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
[poc] add missing rsv functions #1269
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@pradel does this make sense? |
Codecov Report
@@ Coverage Diff @@
## master #1269 +/- ##
==========================================
+ Coverage 64.57% 65.31% +0.74%
==========================================
Files 125 125
Lines 8713 8862 +149
Branches 1901 1944 +43
==========================================
+ Hits 5626 5788 +162
+ Misses 2833 2818 -15
- Partials 254 256 +2
Continue to review full report at Codecov.
|
2360d66
to
b37a490
Compare
b37a490
to
ebe55af
Compare
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.
Always a fan of explicit naming 👍
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.
Really like the Rsv suffix, it will avoid confusion and prevent errors.
ebe55af
to
676d7b0
Compare
676d7b0
to
d55c046
Compare
d55c046
to
a94c266
Compare
scope
references (important for history/context)
usage
The original functions are marked as deprecated and the correct format methods introduced use a suffix of
Rsv
e.g.verifyMessageSignatureRsv(...)
I would propose this deprecating path forward and let stacks-wallet-web switch to
signMessageHashRsv
. The suffix highlights explicitness. I also addedMessageHash
to the function name to further make it clear that a typical use works on a hash (compared to a message/plain-text) and the sign function does not do any previous message hashing here.If folks are against the suffix, I'm also not opposed to
signMessageHash
or a consumer-facingsignMessage
, which would internally use more explicit methods.In a planned overhaul (and major version bump) of the available crypto-helper methods of stacks.js, we will probably refactor this again and remove deprecated functions. Ideally, I would want to see stacks.js only export Clarity-compatible methods (to not confuse developers).