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

Add Begin Wallet to Showcase #1381

Merged
merged 9 commits into from
Jan 13, 2025
Merged

Conversation

francisluz
Copy link
Contributor

@francisluz francisluz commented Jan 11, 2025

👋 Hello there! Welcome. Please follow the steps below to tell us about your contribution.

  1. Please complete a Checklist
  2. Fill in all sections of the template
  3. Click "Create pull request"

Checklist

<-- Please fill the boxes with [x] before submitting a pull request -->

Showcase addition

<-- Provide information for every bullet in the list below. The tags you select must match the tags in your changes to the showcase.js -->

@rphair rphair changed the title Adding Begin Wallet Add Begin Wallet to Showcase Jan 11, 2025
@rphair rphair added the showcase Indicates a PR/issue on showcase label Jan 11, 2025
Copy link
Member

@katomm katomm left a comment

Choose a reason for hiding this comment

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

Welcome Begin Wallet

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

We reviewed this for CIP-0013 extensions; very few wallets have been compliant with that or have fulfilled the goals of CPS-0016: cardano-foundation/CIPs#882 (review)

I am also happy to see open source in the wallet world, but

  1. it's not open source for the whole wallet, but a core set of features... @katomm @rdlrt was there a guideline of how comprehensive the source code coverage has to be for the opensource tag to be applicable?
  2. documentation is as thin as ever. Currently web pages are all copies of each other: a form-letter stub for YouTube videos on each subject which could in turn be considered promotion rather than documentation.

I will keep an eye on whatever the development team further posts here in favour of the submission & will follow the consensus of other reviewers on this one.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

How about resolving the opensource issue (#1381 (comment)) by changing this phrase — vague, promotional, and not unique for wallets — to instead give the readers enough information to put any use of the opensource tag into context: so it would work with or without the tag...

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

(from https://github.com/BeginWallet/begin-core/blob/main/README.md)

You can also verify how we handle and use your Private Key AKA Root Key.

Also we need to be sure — since this sounds like some resource at Begin is "handling" the private key — that this isn't stored or transmitted to a back-end server (in an encrypted state or not) which should be clearly & visibly disclosed in wallet documentation & installation procedure:

If "we" simply means "your" SDK instance running in the wallet — and Begin resources are not actually "handling" it — then I don't see a problem: although @francisluz you might want to address this ambiguity on your SDK home page itself.

cc @Fell-x27 @SebastienGllmt @rdlrt

@francisluz
Copy link
Contributor Author

francisluz commented Jan 12, 2025

(from https://github.com/BeginWallet/begin-core/blob/main/README.md)

You can also verify how we handle and use your Private Key AKA Root Key.

Also we need to be sure — since this sounds like some resource at Begin is "handling" the private key — that this isn't stored or transmitted to a back-end server (in an encrypted state or not) which should be clearly & visibly disclosed in wallet documentation & installation procedure:

If "we" simply means "your" SDK instance running in the wallet — and Begin resources are not actually "handling" it — then I don't see a problem: although @francisluz you might want to address this ambiguity on your SDK home page itself.

cc @Fell-x27 @SebastienGllmt @rdlrt

Hi @rphair, I'd like to confirm that this "we" means the SDK as you said. We do NOT store the private key in our backend.

For more context, Begin Core has been donating to the OpenWallet Foundation and we're in the transition phase. We'll still be the main Maintainer.

Furthermore, I accept the new suggested description and remove the tag. Thanks a log again guys

@francisluz
Copy link
Contributor Author

(from https://github.com/BeginWallet/begin-core/blob/main/README.md)

You can also verify how we handle and use your Private Key AKA Root Key.

Also we need to be sure — since this sounds like some resource at Begin is "handling" the private key — that this isn't stored or transmitted to a back-end server (in an encrypted state or not) which should be clearly & visibly disclosed in wallet documentation & installation procedure:

If "we" simply means "your" SDK instance running in the wallet — and Begin resources are not actually "handling" it — then I don't see a problem: although @francisluz you might want to address this ambiguity on your SDK home page itself.

cc @Fell-x27 @SebastienGllmt @rdlrt

On the reference you put here, I actually didn't know they were doing it and had an issue with plain text. If it would be a case in the future it should be at least Audited by some accredited company. Most importantly be optional so the users decide whether to use such a service or not.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Splendid response @francisluz and thanks for coming on board.

@rphair rphair requested review from rdlrt and os11k January 13, 2025 08:50
@os11k
Copy link
Collaborator

os11k commented Jan 13, 2025

I just noticed that build is failing, I'm reverting my approval for this PR.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

failure to build after latest changes:

          [cause]: Error: Showcase site with title=Begin Wallet contains errors:
          For open-source sites, please add the 'opensource' tag.

Though error message doesn't indicate this, it seems like the converse is also true... that you can't use the source: tag unless the opensource tag is specified. It should build after that's removed & @francisluz you can further ensure your source link is visible on the landing (home) page.

@rphair
Copy link
Collaborator

rphair commented Jan 13, 2025

... OK, that's not doing it either. @francisluz you can modify if you get any ideas & I'll check in local build when I get back to the computer (within the hour).

@francisluz
Copy link
Contributor Author

... OK, that's not doing it either. @francisluz you can modify if you get any ideas & I'll check in local build when I get back to the computer (within the hour).

Entry updated, we need to leave the source = null the property is required. Build looks good.

@rphair rphair merged commit 00bd7b0 into cardano-foundation:staging Jan 13, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
showcase Indicates a PR/issue on showcase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants