tezos-reward-distributor-organization / tezos-reward-distributor

Tezos Reward Distributor (TRD): A reward distribution software for tezos bakers.
https://tezos-reward-distributor-organization.github.io/tezos-reward-distributor/
GNU General Public License v3.0
87 stars 51 forks source link

Cleanup: min_payment_amt #628

Open jdsika opened 1 year ago

jdsika commented 1 year ago

to unstuck #613 I am documenting the things that may need consideration: src/calc/phased_payment_calculator.py 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.

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 https://github.com/tezos-reward-distributor-organization/tezos-reward-distributor/issues/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
Vlad1mir-D commented 1 year ago

Ok, I'm on it, going to open PR this the week.

jdsika commented 1 year ago

Any progress here?

Vlad1mir-D commented 1 year ago

Sorry, didn't have enough time. I'll get everything sorted at the end of this week.

jdsika commented 1 year ago

I will release v11.5 this weekend

jdsika commented 1 year ago

The other PR is also dragging along. I just want to know if I should wait for you here as well?