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

Describe recommended SEP-6 implementations #356

Merged
merged 3 commits into from Jul 26, 2019
Merged

Conversation

tomquisel
Copy link
Contributor

@tomquisel tomquisel commented Jul 25, 2019

Describe recommended wallet and anchor implementations to demystify all the possible ways of implementing SEP-6. See new recommendations section.

Copy link
Member

@accordeiro accordeiro left a comment

Choose a reason for hiding this comment

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

This looks great! Having a general recommendation is a strong addition to SEP6, and helps guide anchors on what they should / shouldn't worry about.

As a general question, I wonder if we should be even more opinionated on the anchor implementation side. More specifically, if we've realized most anchors will require some sort of KYC, signup, and/or other user interactions, should we simply describe the path for implementing the interactive flow, and not worry about the other cases? I think this could make this guide even simpler, without the risk of being less generic, since you can achieve pretty much everything through the interactive flow.

ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
@tomquisel
Copy link
Contributor Author

@accordeiro that's a great point! I'll simplify the anchor section to just describe the interactive flow.

What do you think about applying the same simplification to the wallet section? I'm torn on it, because a number of existing anchors (like apay and naobtc) use the "immediate success" flow, and I'd like for wallets to be able to interact with them as a baseline. But it is more complicated to have the two branches...

@accordeiro
Copy link
Member

That's a good question. My initial thought was what you described: there would be no downsides on asking anchors to only implement the interactive flow, but asking wallets to do the same would make them incompatible with a few anchors.

I also don't love the two branches, but if we want to support these "immediate success" flow anchors as a baseline, it seems like we should keep both approaches as a recommendation for wallets – at least for now.

Maybe we could get in touch with said anchors and start surfacing a migration to the interactive flow, and plan to update the wallet recommendation to solely interactive flow when we have sufficient adoption?

Copy link
Contributor

@tomerweller tomerweller left a comment

Choose a reason for hiding this comment

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

I love this!
Aside from the small notes, I'm with @accordeiro regarding only describing the interactive flow as that's the current standard.

ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
### Basic Wallet Implementation

* Identify assets you want to support manually in advance
* For each asset, use information from its `stellar.toml` and its `/info` endpoint to display useful information to the user like transaction fee structure, types of deposits and withdrawal supported, and details about the anchor like street address.
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the previous point I would change "For each asset" to "For each asset the account has a trustline to"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They might want to deposit something with no trustline (yet), though.

@tomquisel
Copy link
Contributor Author

@rice2000 do you think the formatting is better now? Or should I use headers instead?

@rice2000
Copy link
Contributor

@rice2000 do you think the formatting is better now? Or should I use headers instead?

I think this works great! Aces!

@tomquisel tomquisel merged commit cba2de1 into master Jul 26, 2019
@tomquisel tomquisel deleted the tomquisel-patch-1 branch July 26, 2019 23:29
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.

None yet

4 participants