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: unsigned warning #1067

Merged
merged 2 commits into from
Nov 9, 2022
Merged

feat: unsigned warning #1067

merged 2 commits into from
Nov 9, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Nov 4, 2022

What it solves

Resolves #898

How this PR fixes it

A new warning is shown when a proposed transaction has no signatures.

How to test it

Open /gor:0xF3E977e7Eea1A91ce5B2d5077e5A7195Ae18b722/transactions/tx?id=multisig_0xF3E977e7Eea1A91ce5B2d5077e5A7195Ae18b722_0xc0c5380e113ecff6054c69d02fe7d5ee753db29635548c80794bd028cf614bb5 and observe the warning.

Screenshots

image

@iamacook iamacook requested a review from DiogoSoaress November 4, 2022 15:50
@github-actions
Copy link

github-actions bot commented Nov 4, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 4, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d92d8e3
Status: ✅  Deploy successful!
Preview URL: https://6fc3ecfa.web-core.pages.dev
Branch Preview URL: https://unsigned-warning.web-core.pages.dev

View logs

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

IMHO this warning isn't visible enough. I would write it in big red letters above the transaction. Perhaps in an <Alert>

@katspaugh
Copy link
Member

katspaugh commented Nov 5, 2022

And text-wise, and I would write something like “This transaction is unsigned and could have been created by anyone. To avoid phishing, only sign it if you trust the source of the link.”
Perhaps too grim?

@iamacook
Copy link
Member Author

iamacook commented Nov 7, 2022

IMHO this warning isn't visible enough. I would write it in big red letters above the transaction. Perhaps in an <Alert>

This follows the style of our other warnings. I'll ask design and see what they think.

Edit: @liliiaorlenko said it's ok to keep it as is for now and revisit the entire Warning implementation after "ranking" all warnings we show at a later stage.

@iamacook iamacook requested a review from katspaugh November 7, 2022 08:36
Copy link
Member

@DiogoSoaress DiogoSoaress left a comment

Choose a reason for hiding this comment

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

Code looks good and if this is aligned with designer 👍

I agree with this comment however

IMHO this warning isn't visible enough. I would write it in big red letters above the transaction. Perhaps in an

though an unsigned transaction can come from a friendly source, I think it would be worth to make it more prominent (as suggested) and thus not closing the issue just with this PR.

@iamacook
Copy link
Member Author

iamacook commented Nov 8, 2022

I agree with this comment however

IMHO this warning isn't visible enough. I would write it in big red letters above the transaction. Perhaps in an

though an unsigned transaction can come from a friendly source, I think it would be worth to make it more prominent (as suggested) and thus not closing the issue just with this PR.

After syncing with @liliiaorlenko, we intend to document our transaction list in detail and then reassess all transaction-related notifications thereafter.

@katspaugh
Copy link
Member

I was talking about the Single Tx page btw. The list is safe, we never load untrusted txns there. But anyone can create and share a deeplink.

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.

Show a security warning on the Single Tx page if the tx is unsigned
3 participants