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 diagrams from draw.io #122

Merged
merged 4 commits into from
Jun 11, 2020
Merged

Add diagrams from draw.io #122

merged 4 commits into from
Jun 11, 2020

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Jun 5, 2020

I'm trying to figure out how best to share these in such a way that we
can all contribute to them easily. While integration between draw.io
and GitHub exists, it requires read/write permissions to all ParityTech
repos, which doesn't comfort me. I think having the .drawio file and
the corresponding pictures available in a folder is a good start.

I imagine the workflow would be something like this: Someone wants to
update a diagram, so they go to draw.io and upload the .drawio file
from the repo. They make their changes, download the new .drawio file
and corresponding SVG, and commit their changes to the repo.

I plan to make a Wiki page for showing these diagrams off and possibly
going into more in-depth explanations about certain things.

I'm trying to figure out how best to share these in such a way that we
can all contribute to them easily. While integration between draw.io
and GitHub exists, it requires read/write permissions to all ParityTech
repos, which doesn't comfort me. I think having the .drawio file and
the corresponding pictures available in a folder is a good start.

I imagine the workflow would be something like this: Someone wants to
update a diagram, so they go to draw.io and upload the .drawio file
from the repo. They make their changes, download the new .drawio file
and corresponding SVG, and commit their changes to the repo.
@HCastano HCastano added the A-docs Improvements or additions to documentation label Jun 5, 2020
@tomusdrw
Copy link
Contributor

tomusdrw commented Jun 8, 2020

The workflow looks okay to me for start.

I plan to make a Wiki page for showing these diagrams off and possibly
going into more in-depth explanations about certain things.

We can just have a md file in the repo that embeds the SVG. Or set up the wiki so that it's generated from files in the repo - I'd prefer to have it version controlled.

Comments to the diagrams:

  • bridge-relay.svg
    • I feel like "Substrate Client" and "Ethereum Client" should be two separate blocks, now it kinds of suggest you can have one client that works for both chains.
    • The blocks within sync loop are not verify descriptive, but I guess more elaborate documentation will be included in a text form.
    • The diagram does not have a background set (but that's probably alright)
  • cross-chain-fund-transfer.svg:
    • Nor the diagram neither the boxes have any background. I think it would be good to for the boxes to have white background.
    • My impression was that relay is not responsible for submitting transaction proof, and we make this users responsibility. The events are also not necessary, since we can just look at the transactions itself, not the events. If that's the case then I think we should prepare a tx-proof-submitter relay component for users, so that it can be configured to relay particular kind of transactions. I think initially we can relay the transactions for them (but we have to be careful with tx costs - first relaying might be on us, but subsequent transfers might require topping up our account first (like giving a tip prior to relaying)).
  • ethereum-pallet.svg
    • "Punish Bad Summitter" - typo
  • Some other SVGs are missing boxes backgrounds as well.

Looks pretty good overall, I think adding text descriptions will clear more things up.

The background for each diagram has been removed (now it's transparent)
and the boxes that make up the diagrams have gotten a filled in white
colour. This will make it possible to have these diagrams show up on
non-white backgrounds.
@HCastano
Copy link
Contributor Author

HCastano commented Jun 9, 2020

So I made the background for each diagram transparent (you need to tick a box when you export them from draw.io), and I filled in all the actual diagram boxes to white. I've also dumped the diagrams in an ARCHITECTURE.md file. I'm not super keen on making write ups for them right now, but if you guys think that it should be required as part of this PR I can do it.

Also, regarding this:

My impression was that relay is not responsible for submitting transaction proof, and we make this users responsibility.

I think this is a larger discussion outside the scope of this PR, but it's good that these visualizations can help us get an idea of what we each understand about the project so we can work towards a common consensus :)

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good to me, fine with having write-ups be a separate PR (or even just log an issue, probably a good one for a start for someone).

Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

Had the same concern about cross-chain-fund-transfer.svg - there's no auto-relaying tx proofs now. But as long as it is known, I'm ok with that.

@HCastano HCastano merged commit 36353d1 into master Jun 11, 2020
@HCastano HCastano deleted the hc-add-drawio-diagrams branch June 11, 2020 14:02
svyatonik pushed a commit that referenced this pull request Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants