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 arbitrary message signing #2350

Merged
merged 1 commit into from
May 11, 2022

Conversation

beguene
Copy link
Contributor

@beguene beguene commented Apr 6, 2022

Try out this version of the Hiro Wallet - download extension builds.

closes #1051

Pending Todos:

Decoding message as Clarity Value (see #2387)

signature-test-app.mov

@vercel
Copy link

vercel bot commented Apr 6, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/blockstack/stacks-wallet-web/D9MRHTF5hcVk9wbX3mhYq4TN4U4x
✅ Preview: Failed

@beguene
Copy link
Contributor Author

beguene commented Apr 11, 2022

SigningPopup

@landitus This is the current implementation (just the warning message at the bottom is missing). Let me know if you find any issue.

@landitus
Copy link

@beguene Since we've just updated the designs, lmk when you update them so I can do a review.

@beguene
Copy link
Contributor Author

beguene commented Apr 15, 2022

@landitus
arbitrary-message-signing-collapsed
arbitrary-message-signing-expanded

@landitus
Copy link

landitus commented Apr 19, 2022

Thanks @beguene! I can't run the package cause it's failing, but I can review the screenshots for now.

A few observations:

  • Beware of the excess in spacing (top/bottom padding) in the hash drawer
  • The hash can use the Fira code typography, which is available in the codebase and serves as a monospaced font for code and a lighter color: text.subdued
  • The title "Message" is not needed
  • For the actual message box:
    -- The rounded corners are a bit off. Check that they are 12px
    -- Is the height of the box declared or due to top/bottom padding? I think that when the messages are short, it's nice to have a larger box and when they are longer (like for the tupple) the box should fit it's contents. I think we could declared a min-height property to accomplish that.

@beguene
Copy link
Contributor Author

beguene commented Apr 20, 2022

@landitus Yes you can't use this build yet, I need to fix a few packages first.
Thanks for the comments, I will take into account your observations.

@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from ddc0aca to 7139e26 Compare April 21, 2022 11:14
@vercel
Copy link

vercel bot commented Apr 21, 2022

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

Name Status Preview Updated
stacks-wallet-web ✅ Ready (Inspect) Visit Preview May 11, 2022 at 7:38AM (UTC)

@beguene beguene changed the base branch from dev to remove-username-checking April 21, 2022 11:16
@vercel vercel bot temporarily deployed to Preview April 21, 2022 11:17 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 7139e26 to 54869ee Compare April 21, 2022 11:18
@vercel vercel bot temporarily deployed to Preview April 21, 2022 11:19 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 54869ee to 7e02582 Compare April 21, 2022 11:19
@vercel vercel bot temporarily deployed to Preview April 21, 2022 11:20 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 7e02582 to 4e39c57 Compare April 21, 2022 11:23
@vercel vercel bot temporarily deployed to Preview April 21, 2022 11:24 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 4e39c57 to 78da80e Compare April 21, 2022 11:28
@vercel vercel bot temporarily deployed to Preview April 21, 2022 11:30 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 78da80e to b9ffd3c Compare April 21, 2022 13:08
@vercel vercel bot temporarily deployed to Preview April 21, 2022 13:31 Inactive
@beguene beguene changed the title WIP feat: add arbitrary message signing feat: add arbitrary message signing Apr 21, 2022
@beguene beguene marked this pull request as ready for review April 21, 2022 14:38
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from b9ffd3c to b306e15 Compare April 21, 2022 14:43
@vercel vercel bot temporarily deployed to Preview April 21, 2022 14:50 Inactive
Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

This is going to conflict with my refactor 🙈

src/app/pages/signature-request/components/network-row.tsx Outdated Show resolved Hide resolved
src/app/pages/signature-request/components/network-row.tsx Outdated Show resolved Hide resolved
src/app/store/signatures/requests.hooks.ts Outdated Show resolved Hide resolved
src/background/background.ts Outdated Show resolved Hide resolved
src/app/pages/signature-request/components/sign-action.tsx Outdated Show resolved Hide resolved
src/app/common/actions/finalize-message-signature.ts Outdated Show resolved Hide resolved
src/app/pages/signature-request/components/sign-action.tsx Outdated Show resolved Hide resolved
@landitus
Copy link

@landitus I fixed the issues you mentioned. Let me know if it's fine.

Thanks @beguene. this is looking very good! Just a few more comments I found

CleanShot 2022-04-29 at 16 46 43@2x

@vercel vercel bot temporarily deployed to Preview May 2, 2022 06:24 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch 2 times, most recently from 50b7330 to 728669e Compare May 2, 2022 10:56
@vercel vercel bot temporarily deployed to Preview May 2, 2022 10:58 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 728669e to 5e7f13e Compare May 2, 2022 11:21
@vercel vercel bot temporarily deployed to Preview May 2, 2022 11:22 Inactive
@beguene
Copy link
Contributor Author

beguene commented May 2, 2022

@landitus Thanks, those issues are fixed
sign2

Note that the separator is not exactly like in your design, it does not extend fully to the sides. If it's still an issue, I can fix it in a separate issue.

@Eshwari007
Copy link

Eshwari007 commented May 3, 2022

@beguene Two quick findings/questions:
1-Learn more about signing messages is directing to https://www.hiro.so/
2- Do we want to reword 'to prove you own this account and more' as having more at the end felt like hanging sentence.

Let me know if you want me to create an enhancement ticket for the above.
cc @markmhx

@landitus
Copy link

landitus commented May 3, 2022

Note that the separator is not exactly like in your design, it does not extend full to the sides. If it's still an issue, I can fix it in a separate issue.

I see @beguene. Let's leave for a separate issue if that helps the release. Also, we could improve the padding for the hash box a bit more in that same task. Want me to create a ticket? Thank you.

@markmhendrickson
Copy link
Collaborator

@sjwhitely it appears we do need an FAQ for this functionality after all. Mind drafting and providing its URL for integration? 🙏

@markmhendrickson
Copy link
Collaborator

Do we want to reword 'to prove you own this account and more' as having more at the end felt like hanging sentence.

I'd suggest improving this copy in general to:

By signing this message, you are authorizing "Testing App" to do something specific on your behalf. Only sign messages that you understand from apps that you trust. Learn more

@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 5e7f13e to dae1994 Compare May 5, 2022 11:51
@vercel vercel bot temporarily deployed to Preview May 5, 2022 11:51 Inactive
@beguene
Copy link
Contributor Author

beguene commented May 5, 2022

@Eshwari007 @markmhx I updated the copy.

@Eshwari007
Copy link

@beguene Two quick findings/questions: 1-Learn more about signing messages is directing to https://www.hiro.so/ 2- Do we want to reword 'to prove you own this account and more' as having more at the end felt like hanging sentence.

Let me know if you want me to create an enhancement ticket for the above. cc @markmhx

@beguene The rewording of the dialogue message in arbitrary message is looking good. Will re test the link to 'Learn More' once @sjwhitely updates it.

Thanks

@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from dae1994 to 0da8a1e Compare May 10, 2022 09:48
@vercel vercel bot temporarily deployed to Preview May 10, 2022 09:51 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 0da8a1e to 124b2cb Compare May 10, 2022 13:34
@vercel vercel bot temporarily deployed to Preview May 10, 2022 13:37 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 124b2cb to 7c6aca2 Compare May 11, 2022 07:35
@vercel vercel bot temporarily deployed to Preview May 11, 2022 07:36 Inactive
@beguene beguene force-pushed the feat/arbitrary-message-signing/I1051 branch from 7c6aca2 to 250b7cb Compare May 11, 2022 07:37
@vercel vercel bot temporarily deployed to Preview May 11, 2022 07:38 Inactive
@beguene beguene merged commit cfb28e8 into dev May 11, 2022
@beguene beguene deleted the feat/arbitrary-message-signing/I1051 branch May 11, 2022 11:28
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.

Expose method to sign arbitrary messages
6 participants