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

Payment service: fixed computing payment amounts #982

Merged
merged 3 commits into from
Jan 28, 2021

Conversation

Wiezzel
Copy link

@Wiezzel Wiezzel commented Jan 28, 2021

  1. It was previously falsely assumed that total_amount_accepted for an activity or agreement is equal to the total amount of scheduled payments. That is not true in case of accepting non-binding debit notes (i.e. without due date) which increases total_amount_accepted but doesn't trigger payment. This was fixed by introducing total_amount_scheduled field to both activity and agreement model. This field is updated when a payment is scheduled and it is used to compute the payment amount.
  2. Allocation's remaining amount was being updated when a payment was confirmed not when it was scheduled, thus allowing double-spending from an allocation if a new payment was scheduled before the previous one got confirmed. Now spent & remaining amounts are updated when scheduling payment to prevent double-spending.
  3. Paying zero-amount invoices was causing unnecessary transaction costs. A quick fix has been applied not to schedule payments for zero amount. Although, a proper solution that disallows issuing such invoices in the first place is required.

Fixes golemfactory/yagna-triage#43

@Wiezzel Wiezzel added bug Something isn't working alpha.4 labels Jan 28, 2021
@Wiezzel Wiezzel self-assigned this Jan 28, 2021
@Wiezzel Wiezzel force-pushed the wiezzel/scheduled_amount branch from c8b64c6 to c831e54 Compare January 28, 2021 12:07
Copy link
Contributor

@maaktweluit maaktweluit left a comment

Choose a reason for hiding this comment

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

LGTM!

Tested it with JS:blender example and it works smoothly!

1. It was previously falsely assumed that total_amount_accepted for an
   activity or agreement is equal to the total amount of scheduled
   payments. That is not true in case of accepting non-binding
   debit notes (i.e. without due date) which increases
   total_amount_accepted but doesn't trigger payment. This was fixed
   by introducing total_amount_scheduled field to both activity and
   agreement model. This field is updated when a payment is scheduled
   and it is used to compute the payment amount.
2. Allocation's remaining amount was being updated when a payment was
   confirmed not when it was scheduled, thus allowing double-spending
   from an allocation if a new payment was scheduled before the
   previous one got confirmed. Now spent & remaining amounts are
   updated when scheduling payment to prevent double-spending.
3. Paying zero-amount invoices was causing unnecessary transaction
   costs. A quick fix has been applied not to schedule payments for
   zero amount. Although, a proper solution that disallows issuing such
   invoices in the first place is required.

Signed-off-by: Adam Wierzbicki <[email protected]>
Some migrations require disabling foreign key checks for advanced
table manipulation. Unfortunately, disabling foreign keys within
migration doesn't work correctly.

Signed-off-by: Adam Wierzbicki <[email protected]>
@Wiezzel Wiezzel force-pushed the wiezzel/scheduled_amount branch from c831e54 to db3f329 Compare January 28, 2021 15:24
@Wiezzel Wiezzel marked this pull request as ready for review January 28, 2021 15:24
@Wiezzel Wiezzel requested a review from a team January 28, 2021 15:24
@Wiezzel Wiezzel enabled auto-merge January 28, 2021 15:25
@Wiezzel Wiezzel merged commit 039a826 into release/v0.6 Jan 28, 2021
@tworec tworec deleted the wiezzel/scheduled_amount branch January 29, 2021 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants