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

SEP-31: Add mermaid sequence diagram #1145

Merged
merged 6 commits into from
Mar 8, 2022

Conversation

JakeUrban
Copy link
Contributor

@JakeUrban JakeUrban commented Mar 8, 2022

Adds a "Diagrams" section to SEP-31 and starts with a happy-path sequence diagram.

We've discussed whether or not to include diagrams in our SEPs several times in the past and have historically decided not to do so due to the additional burden of maintaining another document. However, now that Github has added support for mermaid syntax, we can keep the SEP as the single source of truth and edit diagrams directly as markdown.

@JakeUrban JakeUrban closed this Mar 8, 2022
@JakeUrban JakeUrban reopened this Mar 8, 2022
@JakeUrban JakeUrban marked this pull request as ready for review March 8, 2022 20:36
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

🎉

* `v1.5.0`: Deprecate refunded boolean. Add refund object to transaction records. ([#1128](https://github.com/stellar/stellar-protocol/pull/1128))
* `v1.5.1`: Add a sequence diagram for successful transactions using firm quotes. ([#1145](https://github.com/stellar/stellar-protocol/pull/1145))
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this at the top of the change log so the change log is ordered descending? Most recent changes are the most interesting to folks.


This diagram demonstrates the interactions between the entities involved in a successful transaction. Specifically it uses the [Receiving Anchor Asset Conversion](#receiving-anchor-asset-conversion) strategy defined described later in the document.

```mermaid
Copy link
Member

Choose a reason for hiding this comment

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

This diagram looks great. 👏🏻

One thing I noticed is that it renders with a massive blank area at the bottom of the diagram. Do you know why this is? Is this a @github bug, or something to do with how we've defined the diagram do you think?

Here's a screenshot of what I see:
Screen Shot 2022-03-08 at 1 12 31 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the same thing, but I don't see anything wrong with the diagram syntax. Let me google a bit to see if this is a known issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@leighmcculloch leighmcculloch 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, great idea.

I don't think we need to block merging this on the rendering issue. It is likely a GitHub bug. It could be worthwhile creating a discussion thread about this bug at https://github.com/github/feedback/discussions/categories/general-feedback or sending a message to GitHub Support.

@JakeUrban JakeUrban merged commit 623a2d3 into stellar:master Mar 8, 2022
@JakeUrban JakeUrban deleted the add-sep31-mermaid-diagram branch March 8, 2022 22:15
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.

2 participants