Volume dumping restriction manager #292

samparsky · 2018-09-28T18:26:19Z

Please check if the PR fulfills these requirements

The commit message follows our Submission guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

#268

What is the current behavior?

(You can also link to an open issue here)

What is the new behavior?

(Define and describe any new functionality. Clarify if this is a feature change)

Does this PR introduce a breaking change?

No

Any Other information:

satyamakgec · 2018-10-02T17:58:04Z

Hey, @samparsky Should I start reviewing this PR or still you are adding some code in it?

samparsky · 2018-10-02T19:18:36Z

@satyamakgec Currently adding more tests. Will let you know when I am done

samparsky · 2018-10-03T12:55:12Z

@satyamakgec Please kindly review this PR

contracts/modules/TransferManager/VolumeDumpingRestrictionManager.sol

satyamakgec · 2018-10-04T18:05:45Z

@samparsky left some comments on the PR. and you can also improve the test coverage of your contracts although it is good please try to make it 95% - 98%

samparsky · 2018-10-06T21:36:48Z

@satyamakgec I have made the changes suggested above.
The test seems to be failing due to some issue I can't figure out

This returns

$ node_modules/.bin/solidity-coverage

the following error which seems odd, given that truffle compile works perfectly

* Line 1, Column 1
  Syntax error: value, object or array expected.
* Line 1, Column 1
  A valid JSON document must be either an array or an object value.

satyamakgec · 2018-10-08T13:48:49Z

Yes @samparsky we are facing this issue, Once we resolve that then your PR also start building in Travis.

contracts/modules/TransferManager/VolumeDumpingRestrictionManager.sol

samparsky · 2018-10-10T12:09:49Z

@satyamakgec Please review, the changes suggested have been implemented

samparsky · 2018-10-12T12:49:42Z

@satyamakgec Please review, the changes suggested have been implemented

contracts/modules/TransferManager/VolumeDumpingRestrictionManager.sol

adamdossa · 2018-10-12T14:35:17Z

contracts/modules/TransferManager/VolumeDumpingRestrictionManager.sol

+
+            if (_amount <= allowedRemainingAmount){
+                volumeTally[_from][periodId] = volumeTally[_from][periodId].add(_amount);
+                return Result.VALID;


As above should return Result.NA here not Result.VALID

When should Result.VALID be used?

contracts/modules/TransferManager/VolumeDumpingRestrictionManager.sol

samparsky · 2018-10-15T11:18:30Z

@adamdossa @satyamakgec Please review, the changes suggested have been implemented

satyamakgec · 2018-10-15T14:03:36Z

@samparsky Thanks for all your hard work and patience. we are in the process of reviewing the code. it may take a couple of days to finalize the module. So please bear me. I will try to send you an update as soon as possible.

samparsky · 2018-10-18T10:11:16Z

@satyamakgec @adamdossa It would be helpful if this PR can be reviewed & merged.
Merge conflicts occurs when other PRs get merged in. I have had to rebase & fix several times cos of this, it gets frustrating after some time.

satyamakgec

Also, try to increase the branch coverage it is 80 % but we are expecting b/w 95% - 100%

contracts/modules/TransferManager/VolumeDumpingRestrictionManager.sol

contracts/modules/TransferManager/VolumeDumpingRestrictionManagerFactory.sol

contracts/modules/TransferManager/VolumeDumpingRestrictionManager.sol

migrations/2_deploy_contracts.js

scripts/test.sh

samparsky · 2018-10-21T15:12:26Z

@satyamakgec I have implemented the changes you have suggested.
The code coverage for the files added is at 96% & 98%.

VolumeDumpingRestrictionTMFactory.sol - 96%
VolumeDumpingRestrictionTM.sol - 98%

I still have some questions,
Regarding this below, I haven't gotten any clear response from you yet as it doesn't quite agree with the design I was given https://www.lucidchart.com/documents/edit/c96ff64c-5b89-4b9c-985a-01637266bf88/0?shared=true&

This will give you a wrong calculation
ex- A will have the balance of 1000 tokens
percentage allowed - 10% = 100 tokens
First txn deduct the 50 tokens and after first txn A will get the 1000 tokens from B.
for the second transaction(same periodId) calculation will look like this
(1950 + 50 ) * (10)/100 -- it is totally wrong because now it will allow him to deduct 200 tokens instead of more 50 tokens.

It would be awesome to get a response

satyamakgec · 2018-10-23T05:47:51Z

@samparsky Thanks for the changes Yes you are right it is not matching with the design. I will give you more information after the internal discussion on design.

samparsky · 2018-10-24T12:30:07Z

@satyamakgec @adamdossa I am waiting on your decision

satyamakgec · 2018-10-27T13:48:52Z

Hey @samparsky sorry for the delay. The logic looks as -
let’s assume the limit is 10,000 tokens in a rolling 30 days. If SUM (all sales in the last 29 days) + sale requested now < 10,000 then the transaction can proceed, else the transaction reverts. and yes you are doing right as per the lucidchart. Always takes the user current balance in the calculation. But please keep in mind the logic of rolling period which seems off to me in your current code.

samparsky · 2018-10-27T14:01:44Z

@satyamakgec That means the calculation of the limit is correct,

As for the logic of the rolling period is calculated thus

The assumption is there is no rollover of tokens transfer i.e. if you have 10k limit in a period & you only transfer 5k you don't get the remaining 5k rolled over to the next period

endtime - 200
starttime - 100
rollingperiod - 10

currentTime = 101
unit256 periodId = (101-100) / 10 = 0

currentTime = 102
uint256 periodId = (102 -100) / 10 = 0

currentTime = 120
uint256 periodId = (120-100) / 10 = 2

currentTime = 150
uint256 periodId = (150-100) / 10 = 5

It would be great if we can work together to close the PR asap, its taken quite a very long time.

samparsky · 2018-10-29T11:25:41Z

@satyamakgec @adamdossa Still waiting on you

satyamakgec · 2018-10-29T22:08:11Z

Hey we are at Devcon so the coming week will be pretty much busy. I will catch you up after the devcon. Cheers

…

On Mon, Oct 29, 2018, 12:25 Omidiora Samuel ***@***.***> wrote: @satyamakgec <https://github.com/satyamakgec> @adamdossa <https://github.com/adamdossa> Still waiting on you — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#292 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AN5_BVgFNplBi81eBBsugxyI20QexsSaks5upuW4gaJpZM4W_Q-U> .

samparsky · 2018-11-06T06:38:53Z

@satyamakgec Hello, how was Devcon? Hoping we can conclude on this, this week

satyamakgec · 2018-11-08T07:55:54Z

@samparsky Devcon was good and yes we can conclude this week.

contracts/modules/TransferManager/VolumeDumpingRestrictionTM.sol

satyamakgec · 2018-11-08T12:37:30Z

@samparsky please do resolve the above issues and update your branch so I can give this module a green light

…lation

added more tests added movePeriod amounts added changes

samparsky · 2018-11-10T18:15:08Z

@satyamakgec I have implemented the suggested changes. I changed the implementation of the periodId to be more dynamic to allow changing the rollingPeriod without affecting the periodId calculation.

maxsam4 · 2018-11-15T10:32:49Z

Bounty paid. Thanks a lot for your contribution @samparsky .

samparsky · 2018-11-15T10:35:53Z

@maxsam4 Any reason why the PR wasn't merged?

maxsam4 · 2018-11-15T10:47:02Z

@samparsky We have decided to merge this module and #320

It would not have been fair of us to ask you guys to do the required change so we'll be doing them ourselves.
If you want to work on merging them in your free time, feel free and I'll reopen this PR. Thank you for all your effort, we really appreciate your work.

samparsky force-pushed the volume-restriction-manager branch from 6fac7ee to c14925d Compare September 29, 2018 09:05

samparsky force-pushed the volume-restriction-manager branch from b76aa75 to 0937471 Compare October 3, 2018 12:54

gitcoinbot mentioned this pull request Oct 3, 2018

Module Development Task: Volume Restriction (Dumping) #268

Closed

samparsky changed the title ~~[WIP] Volume dumping restriction manager~~ Volume dumping restriction manager Oct 3, 2018

satyamakgec suggested changes Oct 4, 2018

View reviewed changes

samparsky force-pushed the volume-restriction-manager branch from 5222dcf to 628cf87 Compare October 5, 2018 16:40

satyamakgec suggested changes Oct 9, 2018

View reviewed changes

samparsky force-pushed the volume-restriction-manager branch 2 times, most recently from c2028d7 to 1acfa9e Compare October 11, 2018 06:53

adamdossa suggested changes Oct 12, 2018

View reviewed changes

samparsky force-pushed the volume-restriction-manager branch from 1acfa9e to 0dcb580 Compare October 15, 2018 06:22

satyamakgec suggested changes Oct 21, 2018

View reviewed changes

samparsky force-pushed the volume-restriction-manager branch 6 times, most recently from 6b6ca11 to 579c596 Compare October 21, 2018 13:05

satyamakgec suggested changes Nov 8, 2018

View reviewed changes

contracts/modules/TransferManager/VolumeDumpingRestrictionTM.sol Show resolved Hide resolved

contracts/modules/TransferManager/VolumeDumpingRestrictionTM.sol Show resolved Hide resolved

samparsky force-pushed the volume-restriction-manager branch from 883f8e1 to 1157d95 Compare November 10, 2018 09:39

Omidiora Samuel and others added 8 commits November 10, 2018 10:42

added contracts & test file

2d7f289

test changes, added multi methods, tests and changed percentage calcu…

00de503

…lation

remove uneeded changes

7abddb6

remove uneeded changes

b57b097

added more tests

d5dcc8c

fixed a test

de1ec04

added more tests

59dee52

added more tests added movePeriod amounts added changes

modified volume dumping

bb73259

samparsky force-pushed the volume-restriction-manager branch from 1157d95 to bb73259 Compare November 10, 2018 09:44

samparsky added 5 commits November 10, 2018 11:07

test changes

9d2239c

test changes

5bb9f52

test changes

7926b04

fix case

5ef377c

added more test cases

593e743

maxsam4 closed this Nov 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Volume dumping restriction manager #292

Volume dumping restriction manager #292

samparsky commented Sep 28, 2018 •

edited

Loading

satyamakgec commented Oct 2, 2018

samparsky commented Oct 2, 2018

samparsky commented Oct 3, 2018

satyamakgec commented Oct 4, 2018

samparsky commented Oct 6, 2018 •

edited

Loading

satyamakgec commented Oct 8, 2018

samparsky commented Oct 10, 2018 •

edited

Loading

samparsky commented Oct 12, 2018

adamdossa Oct 12, 2018

samparsky Oct 12, 2018

samparsky commented Oct 15, 2018

satyamakgec commented Oct 15, 2018

samparsky commented Oct 18, 2018 •

edited

Loading

satyamakgec left a comment

samparsky commented Oct 21, 2018 •

edited

Loading

satyamakgec commented Oct 23, 2018

samparsky commented Oct 24, 2018

satyamakgec commented Oct 27, 2018

samparsky commented Oct 27, 2018 •

edited

Loading

samparsky commented Oct 29, 2018

satyamakgec commented Oct 29, 2018 via email

samparsky commented Nov 6, 2018

satyamakgec commented Nov 8, 2018

satyamakgec commented Nov 8, 2018

samparsky commented Nov 10, 2018

maxsam4 commented Nov 15, 2018

samparsky commented Nov 15, 2018

maxsam4 commented Nov 15, 2018

Volume dumping restriction manager #292

Volume dumping restriction manager #292

Conversation

samparsky commented Sep 28, 2018 • edited Loading

Please check if the PR fulfills these requirements

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Any Other information:

satyamakgec commented Oct 2, 2018

samparsky commented Oct 2, 2018

samparsky commented Oct 3, 2018

satyamakgec commented Oct 4, 2018

samparsky commented Oct 6, 2018 • edited Loading

satyamakgec commented Oct 8, 2018

samparsky commented Oct 10, 2018 • edited Loading

samparsky commented Oct 12, 2018

adamdossa Oct 12, 2018

Choose a reason for hiding this comment

samparsky Oct 12, 2018

Choose a reason for hiding this comment

samparsky commented Oct 15, 2018

satyamakgec commented Oct 15, 2018

samparsky commented Oct 18, 2018 • edited Loading

satyamakgec left a comment

Choose a reason for hiding this comment

samparsky commented Oct 21, 2018 • edited Loading

satyamakgec commented Oct 23, 2018

samparsky commented Oct 24, 2018

satyamakgec commented Oct 27, 2018

samparsky commented Oct 27, 2018 • edited Loading

samparsky commented Oct 29, 2018

satyamakgec commented Oct 29, 2018 via email

samparsky commented Nov 6, 2018

satyamakgec commented Nov 8, 2018

satyamakgec commented Nov 8, 2018

samparsky commented Nov 10, 2018

maxsam4 commented Nov 15, 2018

samparsky commented Nov 15, 2018

maxsam4 commented Nov 15, 2018

samparsky commented Sep 28, 2018 •

edited

Loading

samparsky commented Oct 6, 2018 •

edited

Loading

samparsky commented Oct 10, 2018 •

edited

Loading

samparsky commented Oct 18, 2018 •

edited

Loading

samparsky commented Oct 21, 2018 •

edited

Loading

samparsky commented Oct 27, 2018 •

edited

Loading