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: initial sov-snap implementation #1

Merged
merged 16 commits into from
Oct 10, 2023
Merged

Conversation

vlopes11
Copy link

@vlopes11 vlopes11 commented Oct 4, 2023

No description provided.

});

case 'signMessage': {
const { message, curve, ...params } = request.params as SignMessageParams;
Copy link
Member

Choose a reason for hiding this comment

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

This destructuring doesn't look like it's behaving as expected. The message isn't being displayed correctly in the snap, and signing seems to be failing
Screenshot 2023-10-04 at 7 44 33 PM

Copy link
Author

Choose a reason for hiding this comment

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

The domain of frontend development hehehe. I developed this under Firefox Beta, and it worked. I'll check for a more compatible way, as this is part of the snap

Copy link
Author

Choose a reason for hiding this comment

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

I tested the last commit with chromium and it worked. Did you use a Metamask Flask for the local test site? Vanilla Metamask doesn't have the functionality required to start the site - however, it works fine with the Snap

Copy link
Member

Choose a reason for hiding this comment

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

Yep, this was with Metamask Flask on Chrome

@preston-evans98
Copy link
Member

preston-evans98 commented Oct 5, 2023

This looks pretty plausible to me overall, but I ran into some errors when I actually tried to run it. Lmk if you're not able to reproduce! (I was on Chrome, MacOS, aarch64)

Copy link
Member

@neysofu neysofu 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! I left some comments. One is about CI testing (it would be nice to keep it enabled, if it doesn't break our setup), the others are about code quality/security and defensive compilerOptions. These are mainly relevant for snap, whereas site looks good to me (and it's intended to be a demo, so that's fine).

Feel free to address here or track in separate issues, as long as we keep an eye on all potential problems. If you want to track these in separate issues just let me know, and I'll approve as-it-is.

Thanks for the research and the work done on this, it's an excellent start!

.github/workflows/build-lint-test.yml Outdated Show resolved Hide resolved
packages/snap/images/icon.svg Outdated Show resolved Hide resolved
packages/snap/src/types.ts Show resolved Hide resolved
@vlopes11 vlopes11 mentioned this pull request Oct 9, 2023
@vlopes11 vlopes11 merged commit 18732d5 into main Oct 10, 2023
4 checks passed
@vlopes11 vlopes11 deleted the vlopes11/feature/metamask-snap branch October 10, 2023 17:33
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.

3 participants