Closed Vlad1mir-D closed 1 year ago
@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!
It is planned for next week! I am still on vacation:))
It is planned for next week! I am still on vacation:))
Oh, didn't mean to bother you. Thanks for this reply!
@jdsika Successfully resolved all notices, thank you for your time!
@jdsika What do you think? Do I need to fix/improve any other parts?
@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!
I invited you to the org in order to create branches with "write rights"
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.
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.
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)
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 @vkresch Hi! What's left for this MR?
@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.
@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.
@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 https://github.com/tezos-reward-distributor-organization/tezos-reward-distributor/pull/613#discussion_r977091140 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.
/unstuck
:smiley_cat:
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 topayment_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 forcalculate()
incalc/phased_payment_calculator.py
and this seems to be the right choice in my opinion.Performed tests:
Check list:
Work effort: 12h