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

Cleanup: min_payment_amt #628

Open
jdsika opened this issue Nov 18, 2022 · 5 comments
Open

Cleanup: min_payment_amt #628

jdsika opened this issue Nov 18, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@jdsika
Copy link
Contributor

jdsika commented Nov 18, 2022

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 #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
@jdsika jdsika added the enhancement New feature or request label Nov 18, 2022
@Vlad1mir-D
Copy link
Contributor

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

@jdsika
Copy link
Contributor Author

jdsika commented Jan 24, 2023

Any progress here?

@Vlad1mir-D
Copy link
Contributor

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

@jdsika
Copy link
Contributor Author

jdsika commented Jan 31, 2023

I will release v11.5 this weekend

@jdsika
Copy link
Contributor Author

jdsika commented Feb 17, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants