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

fix: support SIP-018 for structured data signing #2484

Merged
merged 1 commit into from Jun 23, 2022

Conversation

beguene
Copy link
Contributor

@beguene beguene commented Jun 10, 2022

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

This PR implements the updated version of SIP-018 stacksgov/sips#57
and fixes the following too

  • return all signatures in RSV format
  • display correct hash in the UI

closes #2443
closes #2460
closes #2419
closes #2463

@vercel
Copy link

vercel bot commented Jun 10, 2022

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

Name Status Preview Updated
stacks-wallet-web ❌ Failed (Inspect) Jun 23, 2022 at 9:11AM (UTC)

@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from 304c4e7 to 9f20d9e Compare June 10, 2022 13:10
@vercel vercel bot temporarily deployed to Preview June 10, 2022 13:13 Inactive
@beguene
Copy link
Contributor Author

beguene commented Jun 10, 2022

@landitus Now the structured message also has a domain value like this

  const domain = tupleCV({
    name: stringAsciiCV('hiro.so'),
    version: stringAsciiCV('1.0.0'),
    'chain-id': uintCV(1),
  }); 

domain

I added a message and domain header, but we could do a bit better. Let me know if you have something better.

@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from 9f20d9e to 1627580 Compare June 10, 2022 13:28
@beguene beguene marked this pull request as ready for review June 10, 2022 13:31
@vercel vercel bot temporarily deployed to Preview June 10, 2022 13:31 Inactive
@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from 1627580 to 1bc3e18 Compare June 10, 2022 13:46
@vercel vercel bot temporarily deployed to Preview June 10, 2022 13:49 Inactive
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/app/pages/signature-request/signature-request.tsx Outdated Show resolved Hide resolved
@MarvinJanssen
Copy link
Contributor

This (very) old concept drawing displayed the app name and version on the top. Maybe something like that is useful for the domain object? Additionally, perhaps it could show a suffix of (mainnet) or (testnet) based on the chain-id property.

SIP018 display concept

@beguene
Copy link
Contributor Author

beguene commented Jun 13, 2022

thanks @MarvinJanssen I will try something like that.

@vercel vercel bot temporarily deployed to Preview June 15, 2022 13:17 Inactive
@beguene
Copy link
Contributor Author

beguene commented Jun 15, 2022

updated popup
structured1

@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from f10d550 to 9cb30fb Compare June 15, 2022 13:57
@vercel vercel bot temporarily deployed to Preview June 15, 2022 14:00 Inactive
@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from 9cb30fb to 1c209c7 Compare June 17, 2022 06:21
@vercel vercel bot temporarily deployed to Preview June 17, 2022 06:24 Inactive
@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from 1c209c7 to a108941 Compare June 21, 2022 13:15
@vercel vercel bot temporarily deployed to Preview June 21, 2022 13:18 Inactive
@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from a108941 to b7d9058 Compare June 21, 2022 13:23
@vercel vercel bot temporarily deployed to Preview June 21, 2022 13:26 Inactive
@markmhendrickson
Copy link
Collaborator

@beguene is this awaiting a design integration review from @landitus?

@beguene
Copy link
Contributor Author

beguene commented Jun 22, 2022

@markmhx No I am working on those test unit issues
https://github.com/hirosystems/stacks-wallet-web/runs/6985575450?check_suite_focus=true#step:5:825

The upgrade to the latest connect-ui v5.5.1 fails many tests. Those connect changes are not related to AMS but AMS support in connect is built on top of it, so the upgrade is necessary.
This should be the last thing pending for this PR.

@beguene
Copy link
Contributor Author

beguene commented Jun 22, 2022

@janniks I upgraded our connect to the latest version which includes your latest changes hirosystems/connect#234
It's failing many unit tests in the web-wallet via jest.
https://github.com/hirosystems/stacks-wallet-web/runs/6985575450?check_suite_focus=true#step:5:825
It looks like node_modules/@stacks/connect/dist/connect.cjs.development.js:13:20 is loading connect-modal.js via import instead of loading the connect-modal.cjs file which is not co-located next to the .js but is in the cjs folder instead.
I am trying to find a solution for the web wallet with some custom jest config
like

   moduleNameMapper: {
     '@stacks/connect-ui/dist/components/connect-modal':
       '<rootDir>/node_modules/@stacks/connect/node_modules/@stacks/connect-ui/dist/cjs/connect-modal.cjs.entry.js',
  },

without success.

It seems that connect should package it with the right import rather than having to change our web-wallet jest config.
I saw that you are working on this hirosystems/connect#238
Is it related ? Is there a solution that I can use right now ?

@janniks
Copy link
Contributor

janniks commented Jun 22, 2022

Yep, I think I'll revert the change and add as a separate module. Will ping you later with progress.

@beguene
Copy link
Contributor Author

beguene commented Jun 22, 2022

@timstackblock This is ready for QA. There are special button in test-app to test the different types of message.
cc: @markmhx @andresgalante

@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from b7d9058 to aa66d20 Compare June 23, 2022 05:51
@vercel vercel bot temporarily deployed to Preview June 23, 2022 05:54 Inactive
@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from aa66d20 to bafde08 Compare June 23, 2022 06:02
@vercel vercel bot temporarily deployed to Preview June 23, 2022 06:03 Inactive
@beguene beguene force-pushed the feat/add-sip-018-sign-structured-data/I2460 branch from bafde08 to 15cf471 Compare June 23, 2022 09:09
return all signatures in RSV format
display correct hash in the UI
closes #2443
closes #2460
closes #2419
@vercel vercel bot temporarily deployed to Preview June 23, 2022 09:11 Inactive
@beguene
Copy link
Contributor Author

beguene commented Jun 23, 2022

@fbwoolf There is nothing left for me to do on this PR. I will let you merge it whenever you need it.

@fbwoolf
Copy link
Contributor

fbwoolf commented Jun 23, 2022

@beguene do all of the linked issues need to be QA'd by me here before merging or do you feel confident it can be merged and then tested with a release PR?

@beguene
Copy link
Contributor Author

beguene commented Jun 23, 2022

@fbwoolf It's up to you, you can maybe quickly try it, there are some buttons I added in the test-app.

@fbwoolf fbwoolf merged commit 8087cd6 into dev Jun 23, 2022
@fbwoolf fbwoolf deleted the feat/add-sip-018-sign-structured-data/I2460 branch June 23, 2022 17:01
@landitus
Copy link

updated popup structured1

Sorry I missed this @beguene! Yep, those headers will do the trick just fine 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants