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

pm: tx cost checks #1751

Merged
merged 2 commits into from
Feb 2, 2021
Merged

pm: tx cost checks #1751

merged 2 commits into from
Feb 2, 2021

Conversation

yondonfu
Copy link
Member

@yondonfu yondonfu commented Feb 1, 2021

What does this pull request do? Explain your changes. (required)

This PR adds checks on the orchestrator to:

  • Ensure that the estimated ticket redemption tx cost can be covered by the advertised ticket face value. At the moment, when the default ticket face value is higher than the broadcaster's max float (derived from its reserve), the ticket face value is adjusted to match the broadcaster's max float. A check is added to require that the broadcaster's max float >= the estimated ticket redemption tx cost
  • Ensure that tickets are only redeemed if their face value would cover the estimated ticket redemption tx cost

This PR focuses on adding the above missing checks and leaves further configuration to be addressed separately.

See #1603 for more details.

Specific updates (required)

See commit history.

How did you test each of these updates (required)

Added unit tests.

Checklist:

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

Copy link
Contributor

@darkdarkdragon darkdarkdragon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jailuthra jailuthra left a comment

Choose a reason for hiding this comment

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

LGTM as well!

(although I do not fully understand the details of ticket redemption, but the changes and tests seem to match the solution described in #1603)

@yondonfu yondonfu merged commit 6b1fca4 into master Feb 2, 2021
@yondonfu yondonfu deleted the yf/tx-cost-face-value branch February 2, 2021 23:37
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.

3 participants