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

doc: Add Redeemer docs #1535

Merged
merged 5 commits into from
Jul 1, 2020
Merged

doc: Add Redeemer docs #1535

merged 5 commits into from
Jul 1, 2020

Conversation

kyriediculous
Copy link
Contributor

What does this pull request do? Explain your changes. (required)
This PR adds documentation for the Redeemer service and ticketQueue table.

Does this pull request close any open issues?
Fixes #1532

Checklist:

  • README and other documentation updated
  • Node runs in OSX and devenv
  • All tests in ./test.sh pass

@kyriediculous kyriediculous requested review from j0sh and yondonfu June 9, 2020 14:23
@kyriediculous kyriediculous changed the title Nv/redeemer doc doc: Add Redeemer docs Jun 9, 2020
Copy link
Collaborator

@j0sh j0sh left a comment

Choose a reason for hiding this comment

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

It would be good to also have usage instructions, maybe after the introduction, with recommended CLI flags for both the redeemer service and the orchestrator node.


1. The `ticketQueue` is a loop that runs everytime a new block is seen. It will then pop tickets off the queue starting with the oldest ticket first, and sends it to the `LocalSenderMonitor` for redemption if the `recipientRand` for the ticket has expired.

2. When the `LocalSenderMonitor` receives a ticket from the `ticketQueue` it will substract `ticket.faceValue` from the outstanding `maxFloat` as long as the ticket is in limbo.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clarify "in limbo" ? Would it be clearer to say "not yet confirmed on the blockchain" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pending between the Redeemer and the ethereum node

This can be either pending on-chain or an HTTP request to the eth node that has some latency to it.

@kyriediculous
Copy link
Contributor Author

this branch has been force pushed with changes in accordance with #1513 (comment)

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

Changes look good to me. 👍 for a rebase from me as long as comments from other reviewers are addressed.


8. The local cache for `sender` will be cleaned up if is not requested for 5 minutes.

![Ticket Flow](./assets/redeemer/ticketflow.png)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really like these sequence diagrams, but 200-300 KB is kind of large. I suppose if we're going to be doing a lot more of these types of checkins, we should look at a blob storage extension such as git-lfs (which Github supports).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to include this in this PR? I think this is the only remaining issue for this PR, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to you, but I'm generally mindful of repo growth especially with large binary blobs in Git. Another option: since the source for these diagrams has been checked in and can be reproduced [1], we could upload the images themselves to Github (drag+drop the image in this text box) and link within the docs?

[1] Maybe add a comment to the source(s) noting which tools / service can interpret the code for these diagrams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now using git lfs for .png files

Nico Vergauwen and others added 5 commits July 1, 2020 20:03
# This is the 1st commit message:

doc: redeemer sequence diagrams source

# This is the commit message #2:

doc: redeemer sequence diagrams
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM

@kyriediculous kyriediculous merged commit e631eef into master Jul 1, 2020
@kyriediculous kyriediculous deleted the nv/redeemer-doc branch July 1, 2020 21:03
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.

Ticket Redemption Service docs
3 participants