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

Update wallet listing policy #6438

Merged
merged 56 commits into from
Jun 1, 2022
Merged

Update wallet listing policy #6438

merged 56 commits into from
Jun 1, 2022

Conversation

samajammin
Copy link
Member

@samajammin samajammin commented May 23, 2022

Description

  • Update wallet suggestion issue template
  • Update our wallet listing policy

Preview of form

https://github.com/ethereum/ethereum-org-website/blob/wallet-listing-policy/.github/ISSUE_TEMPLATE/suggest_wallet.yaml

Related Issue

#6434

@github-actions github-actions bot added the tooling 🔧 Changes related to tooling of the project label May 23, 2022

<!-- Please provide a URL to the documentation -->

**Is your wallet security tested? Please explain security measures i.e. security audit, internal security team, or some other method.**
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can remove this - this is covered in the security section:

Please describe the measures taken to ensure the wallet's security and provide documentation wherever possible


<!-- Can a user swap ETH for other tokens from within a screen in the wallet? -->
**Does the wallet support multi-chain networks?**
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
**Does the wallet support multi-chain networks?**
**Does the wallet support layer 2 networks?**

Curious what folks think of "layer 2" vs. "multi-chain"? Or should we include both?

Ultimately I suspect L2 is what we care about. Users may want other EVM chains. There could also be user benefit in e.g. Bitcoin support in a wallet (like Ledger does).

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that we don't care about alt L1s like AVAX here but what about sidechains like Polygon?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a better question: which L2s do you support? e.g. Argent supports zkSync but not Optimism. But Metamask supports custom RPCs.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I see L2 and multi-chain as two different feature sets... one is contained within the Ethereum security bubble, the other is many different security bubbles.

Yes, in general, it's about which networks the wallet can connect to... but someone who uses Ethereum may reasonably want to find a wallet that supports L2s so they can inherit Ethereum's L1 security and still afford gas.

Meanwhile, someone who uses many cryptocurrencies on many chains care that their wallet also supports these other chains.

So my vote would be to include both. We should ask what non-EVM chains are compatible, and also what Ethereum L2s are compatible, imo.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the notion here, but my pushback would be from our discussions of what layer 2's are. I think a user would want to look at validiums or optimistic chains as well, which I would say still fall under the bundle of scaling Ethereum, but we don't classify it as a layer 2. That being said, I think that is more captured in the RPC importing question. Down for a discussion, but I think I agree with this suggestion.

@gatsby-cloud
Copy link

gatsby-cloud bot commented May 23, 2022

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 22m

Performance

Lighthouse report

Metric Score
Performance 🔶 20
Accessibility 💚 100
Best Practices 💚 100
SEO 💚 92

🔗 View full report


<!-- Can a user set a limit for transfers to protect their assets? -->
**Does the wallet support staking directly?**
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder how important this feature is given that users could just swap for liquid staking tokens?

Copy link
Member

Choose a reason for hiding this comment

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

Probably less important. There is still a difference between adding more ETH to the pool, compared to just buying someone elses stake, but I hear ya. I think it's still worth leaving this in.

@ethereum ethereum deleted a comment May 24, 2022
.github/ISSUE_TEMPLATE/suggest_wallet.md Outdated Show resolved Hide resolved

<!-- Please provide a URL to the documentation -->

**Is your wallet security tested? Please explain security measures i.e. security audit, internal security team, or some other method.**
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be more explicit here. People often fluff this section with irrelevant information.

**Has the wallet's smart contract code been audited?**

<!-- If yes, provide a link to any audits. -->

**Does the wallet have an internal security team?**

<!-- If yes, please provide details. -->

**Any other security testing that should be noted?**

<!-- Please note any other security precautions taken. -->

The final point is potentially unnecessary but I think that it might help inform future iterations of the listing criteria.

Copy link
Contributor

Choose a reason for hiding this comment

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

@corwintines thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Will put this in the security section.

.github/ISSUE_TEMPLATE/suggest_wallet.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/suggest_wallet.md Outdated Show resolved Hide resolved

<!-- Can a user swap ETH for other tokens from within a screen in the wallet? -->
**Does the wallet support multi-chain networks?**
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that we don't care about alt L1s like AVAX here but what about sidechains like Polygon?

.github/ISSUE_TEMPLATE/suggest_wallet.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/suggest_wallet.md Outdated Show resolved Hide resolved

**What social links are there for the project?**

<!-- Please provide social links for the wallet (Discord, Twitter, etc.) -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to use this information? We should probably ask for exactly what we want if we are.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see this explicitly in the design right now. @nloureiro Was there a plan to include these?

For comparison to other areas of the site, we have the socials in the staking-products.json for each product, but we ended up not adding these to the product cards that get rendered. Agree if we decide to not include this in the design then we should probably remove the question.

Copy link
Member

Choose a reason for hiding this comment

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

Im ok if these arent in v1 of this release, but I do think at some point showing this information would be ideal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Im ok if these arent in v1 of this release, but I do think at some point showing this information would be ideal.

Then perhaps we ask explicitly? It'll save a bunch of time in the future.

Discord server:
Twitter:
what else?

.github/ISSUE_TEMPLATE/suggest_wallet.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/suggest_wallet.md Outdated Show resolved Hide resolved

<!-- Please explain any security measures you have taken to ensure your wallet is secure -->

**When did your wallet go live to users?**

<!-- Please provide an exact or approximate date when your wallet was usable by the public -->
<!-- Please provide a date when your wallet was usable by the public. Please provide some user metrics for how many users are using this wallet. -->

**Does your wallet have an active development team?**
Copy link
Member

Choose a reason for hiding this comment

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

Open question for discussion: Should an active development team be noted as an absolute requirement?
Also, what defines active?

My take is this should be a requirement to keep users safe. Ethereum is still relatively young and has many upgrades being rolled out, and listed wallets should have developers maintaining the codebase and tending to bug reports.

Not sure what the cutoff should be though for an "inactive" project... Perhaps either an official announcement, or no code commits for > 6 mos?

Copy link
Member

Choose a reason for hiding this comment

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

I do think an active development team is a requirement. Just last year there was the EIP-1559 upgrade that required wallets to upgrade as a single example where there are changes that wallets would need to support. If anything this is almost the main requirement out of all of this.

@corwintines corwintines marked this pull request as ready for review May 27, 2022 16:40
@corwintines corwintines requested a review from pettinarip as a code owner May 27, 2022 16:40
@corwintines
Copy link
Member

Note: I can remove the markdown version of this if we are content with use the yaml forms

@wackerow
Copy link
Member

The yaml looks great, really nice upgrade for those going through this. Would approve, but I assume we want to update the listing policy on the site with this PR as well... should this be in draft in the meantime?

@corwintines
Copy link
Member

Good catch @wackerow , ill update that

@corwintines
Copy link
Member

Actually was looking into the suggest product page, it just links out to https://github.com/ethereum/ethereum-org-website/issues/new/choose. I think we are ok in this PR. I do think we will make a separate wallet page as we get to the listing policy this sprint.

@corwintines corwintines merged commit cb11aab into dev Jun 1, 2022
@corwintines corwintines deleted the wallet-listing-policy branch June 1, 2022 15:02
@samajammin samajammin mentioned this pull request Jun 1, 2022
2 tasks
@wackerow wackerow mentioned this pull request Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling 🔧 Changes related to tooling of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants