Add min_payment_amt configuration option to allow setting minimum payment amount (#612)

[Vlad1mir-D]

Feature: Add min_payment_amt configuration option to allow setting minimum payment amount (#612)

---

name: Pull Request
about: Create a pull request to make a contribution
labels: enhancement

---

IMPORTANT NOTICE:
I read and understood the guidelines for contributions to the TRD. The contribution may qualify for being compensated by the TRD grant if approved by the maintainers.

This PR resolves the issue 612. The following steps were performed:

• Solution: Added support for min_payment_amt configuration option similar to payment_requirements.minimum_amount option which is implemented in the Breadcrumbs.

• Implementation: At first I did attempt to implement this option in calc/calculate_phase_final.py but went nowhere. Then I realized it will be much easier to make a recursive call for calculate() in calc/phased_payment_calculator.py and this seems to be the right choice in my opinion.

• Performed tests:

  • Executed simulation runs for cycles 1-518 and compared results with calculations made without this feature being enabled
  • Using in production starting from cycle 500

• Check list:

  • I extended the Github Actions CI test units with the corresponding tests for this new feature (if needed).
  • I extended the Sphinx documentation (if needed).
  • I extended the help section (if needed).
  • I changed the README file (if needed).
  • I created a new issue if there is further work left to be done (if needed).

Work effort: 12h

[Vlad1mir-D]

@jdsika @nicolasochem @vkresch
Your time and contribution to the development of TRD are very valuable so I will greatly appreciate for the review of this MR.
Thank you!

[jdsika]

It is planned for next week! I am still on vacation:))

[Vlad1mir-D]

It is planned for next week! I am still on vacation:))

Oh, didn't mean to bother you. Thanks for this reply!

[Vlad1mir-D]

@jdsika Successfully resolved all notices, thank you for your time!

[Vlad1mir-D]

@jdsika What do you think? Do I need to fix/improve any other parts?

[Vlad1mir-D]

@jdsika I understand that TRD is an open-source software and there are no obligations exist for maintainers but I have a bunch of features developed for TRD and would like to donate them properly to the TRD community.
I'm not attempting to push you in any way but I want to know if there are some shortcuts I can use to speed-up the MR review process.
Thank you for your time!

[jdsika]

I invited you to the org in order to create branches with "write rights"

[jdsika]

I am having trouble understanding the "inherits behavior [...]" part. What exactly is inherited here? I am setting a value elsewhere and it is copied to min_payment_amt?

Btw I think the abbreviations like "amt" for "amount" are a curse in the code. In the config we are basically forced to use it due to consistency. I try to use proper names in the code where possible... this is more a general remark rather then a request to change anything as you are consistent with the rest of the namings here

[Vlad1mir-D]

I am having trouble understanding the "inherits behavior [...]" part. What exactly is inherited here? I am setting a value elsewhere and it is copied to min_payment_amt?

"inherits behavior" means there are no separate keyword like mindelegation available for "min payment threshold" in the rules_map allowing to set TOF/TOB/TOE specifically for min_payment_amt case so if payment excluded because of minimum payment threshold it will follow the rules set for mindelegation.

I foresee two options here:

• Put extended clarification into the documentation so it will be more or less obvious that min_payment_amt simply manipulates min_delegation_amt internally, follows all rules set for mindelegation in the rules_map and calculation won't reflect separate cause for payment excluded due to the min_payment_amt threshold.
• Implement separate minpayment key for the rules_map and add a separate exclusion reason which will properly reflect exclusion reason in the calculation report. Initially while taking another approach for min_payment_amt I did implement all that stuff but then realized it could add unnecessary complexity to the configuration and decided to drop it in favor of making easier to review final implementation MR.

What do you think? Which is the better way to proceed?

[jdsika]

hm, I am not really a fan of implicit behavior - but the complexity is already high. I think I would opt for the minpayment key anyway and extend the docs for configuration for the user. It is also the thing that made me hesitate it he recursive loop that the min delegation is manipulated!
In addition the payment report is already incomplete (see #576 ) and I would like that fixed. Another puzzle with a double use of a keyword with implicit behaviour might not help there? What do you think?

[jdsika]

I understand correctly that otherwise the avoided payments would be marked with "min_delegation_amt" in the report? Do you think it is a false statement? Are there situations in which a min_payment_amt and a min_delegation_amt are not connected?
Thinking about it we could also couple the explanations of both configurations, write them under each other and explain the connection. In the morning I am leaning towards this :)

[Vlad1mir-D]

@jdsika

It is also the thing that made me hesitate it he recursive loop that the min delegation is manipulated!

I can attempt to fix https://github.com/Vlad1mir-D/tezos-reward-distributor/tree/minpayment-phased and implement this part without recursion but honestly I don't think there is any reason to feel any hesitation about recursion.

In addition the payment report is already incomplete (see #576 ) and I would like that fixed.

I'm not using dry-run mode because currently it requires an access to the signer and this destroys the whole idea of "dry run" as I'm afraid to provide an access to signer. So I can't comment this issue.

Another puzzle with a double use of a keyword with implicit behaviour might not help there? What do you think?

I'm not sure what you mean by "help" so I think it definitely won't help with this thing :)

I understand correctly that otherwise the avoided payments would be marked with "min_delegation_amt" in the report?

In current implementation - yes, they will be marked with min_delegation_amt.

Do you think it is a false statement?

I would say this could be a confusing statement but there many other parts in TRD that's really confusing to me and this is among others is the reason I decided not to add an additional keyword.

Are there situations in which a min_payment_amt and a min_delegation_amt are not connected?

What do you mean by "connected"? There could be situations where both min_payment_amt and min_delegation_amt will be defined in config but only one of them in current implementation could "win" (i.e. exclude most of the payments as the exclusion starts from the bottom) so I don't see any issues except confusion such report could represent but it could be addressed in the documentation.

Thinking about it we could also couple the explanations of both configurations, write them under each other and explain the connection. In the morning I am leaning towards this :)

Well, it's possible to keep both reasons in the report, i.e. when payment is excluded due to one of the minpayment or mindelegation - there will be only one notice and when payment excluded by both thresholds there will be two notices.

To summarize:

1. Add an additional keyword minpayment to the rules_map with the same options which currently available for mindelegation
2. Keep both mindelegation and minpayment exclusion reasons in the report whenever payment get excluded

Are these two things going to be enough for this MR to be included into the master branch?
Thanks!

[Vlad1mir-D]

@jdsika What do you think?

[Vlad1mir-D]

I invited you to the org in order to create branches with "write rights"

Thanks, I'll follow a basic procedure for all stuff I currently have in my TRD wrapper and try to move them into TRD one by one.

[Vlad1mir-D]

Btw I think the abbreviations like "amt" for "amount" are a curse in the code. In the config we are basically forced to use it due to consistency. I try to use proper names in the code where possible... this is more a general remark rather then a request to change anything as you are consistent with the rest of the namings here

Noted. I attempted to use the same abbreviations everywhere but will use full length variable names in further features.

[nicolasochem]

I'm not using dry-run mode because currently it requires an access to the signer and this destroys the whole idea of "dry run" as I'm afraid to provide an access to signer. So I can't comment this issue.

Hi! A dry-run payout is a payout that has been calculated, signed, but not broadcasted. We exercise as much of the stack as possible (because it's useful) except we never inject the operation. There is no reason to be afraid (unless you are touching dry-run logic itself)

[jdsika]

I'm not using dry-run mode because currently it requires an access to the signer and this destroys the whole idea of "dry run" as I'm afraid to provide an access to signer. So I can't comment this issue.

Hi! A dry-run payout is a payout that has been calculated, signed, but not broadcasted. We exercise as much of the stack as possible (because it's useful) except we never inject the operation. There is no reason to be afraid (unless you are touching dry-run logic itself)

#615 should have just removed that feature

[Vlad1mir-D]

@jdsika @vkresch Hi! What's left for this MR?

[vkresch]

@Vlad1mir-D thanks for the PR. Sorry for the long delay. I will review and test this PR this week. It would be easier to review the code if we could see a regression test e.g. test_min_payment_amount.py in the regression folder which tests your feature. I appreciate the detailed documentation you did!!#

Update: Will review the PR this Thursday (13.10). Sorry for the inconvinience I am having a busy week.

[Vlad1mir-D]

@vkresch It would be a pleasure to get a final list of notices that prevents this MR from merging until addressed as I'm really eager to donate all of changes I made in my own fork to the TRD community.

[Vlad1mir-D]

@vkresch

It would be easier to review the code if we could see a regression test e.g. test_min_payment_amount.py in the regression folder which tests your feature.

Because there is no response for #613 (comment) it would be better to get all notices at once as current implementation may change so do the tests and I prefer not to re-implement something multiple times if it's possible not to do so.

[Vlad1mir-D]

/unstuck 😺

[vkresch]

Thanks for the feature and sorry for the long review delay from my side. The implementation is good and the idea with the recalculation is a good example of code reusage :). The min payment is by default 0 if not defined in a config thus the feature is backwards compatible. I have just some minor change request and after that it can be merged. Thanks again and sorry for the inconvenience.

Review effort: 1h

[vkresch]

Please remove the return statement and the rerun variable since it is redundant. The function calculate returns anyway with reward_logs and the total reward amount. Ofc it sorts and logs in between but it should be fine.

[vkresch]

rerun is not needed

Created issue