-
Notifications
You must be signed in to change notification settings - Fork 160
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
Single Trade Volume Restriction #262
Single Trade Volume Restriction #262
Conversation
Thanks @subramanianv for starting to work on this. Let @adamdossa @satyamakgec or myself know if you have any questions. Seems like you are on the right direction so far ;) |
|
||
if(specialTransferLimitWallets[_from]) { | ||
if(globalTransferLimitInPercentage) { | ||
validTransfer = (_amount.mul(100).div(ISecurityToken(securityToken).totalSupply())) <= specialTransferLimits[_from]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error in the calculation. You are giving the margin of 0.9 %. Suppose if the left size has the 15.7% and right side have the 15 % value then it still gets true
instead of false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satyamakgec I don't fully understand your comment. Is your comment because of this warning from the solidity docs ? Division on integer literals used to truncate in earlier versions, but it will now convert into a rational number, i.e. 5 / 2 is not equal to 2, but to 2.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also From PercentageTransferManager
if(newBalance.mul(10**uint256(ISecurityToken(securityToken).decimals())).div(ISecurityToken(securityToken).totalSupply()) > maxHolderPercentage) { return Result.INVALID; }
Shouldn't newBalance be multipled by 10^18 explicitly instead of 10^decimals as maxHolderPercentage is multipled by (10^18/100) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so because by default securitytoken have 18 decimals so in case of PercentageTransferManager
it always give you the value right percentage calculation.
newBalance = x * 10^d
totalSupply = y * 10^d
maxHolderPercentage = z * 10^d
so it looks like this - (x* 10^d * 10^d) / y* 10^d > (z * 10^d)/100 ... where d = 18
After calculation you can easily compare ((x * 10^18)/y) > (z* 10^16)
But in your case you are doing this
amount = a* 10^d
totalSupply = b* 10^d
Limit= c* 10^d (case 1)
so it will look like this - (a* 10^d * 100)/(b* 10^d) <= c* 10^d .... where d = 18
so your left hand side is always less than the right hand side.
Limit = c (case 2)
in this condition - (155000 * 10^d * 100) / (1000000 * 10^d) <= 15 .... a= 155000, b = 1000000, c= 15 and d =18.
After calculation it should be 15.5 <= 15 (and it will returns true as per my test on remix with 0.4.24 and 0.4.25 solidity version).
and using case 2 you also can't cover the limits in decimal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satyamakgec Thanks for the reply. The first case, which I haven't implemented is missing a multiplication by 10^18. it should be (a* 10^d * 10^18)/(b* 10^d) <= c* 10^16
. I will implement this formula/calculation.
In the PercentageTransferManager
- the math should be (x* 10^d * 10^18) / y* 10^d > (z * 10^18)/100. The multiplication should be explicitly done by 10^18. I think this would work for security token of any decimals, not just tokens with 18 decimals
@subramanianv I think the best time to review the code when you have done with the test cases and all the specs get covered. |
Hi @subramanianv thanks for submitting this PR. Could you please make it against the |
e588776
to
c87b5c2
Compare
@pabloruiz55 I made the PR against development-1.5.0. The PR is ready for review :) |
*/ | ||
function configure(bool _isTransferLimitInPercentage, uint256 _globalTransferLimitInPercentageOrToken) public onlyFactory { | ||
require(_globalTransferLimitInPercentageOrToken > 0, "global transfer limit has to greater than 0"); | ||
isTransferLimitInPercentage = _isTransferLimitInPercentage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are dealing with percentage require it to be <= 100
* @param _newGlobalTransferLimitInPercentage new transfer limit in percentage. Multiple the percentage by 10^16. Eg 22% will be 22*10^16 | ||
*/ | ||
function changeGlobalLimitInPercentage(uint256 _newGlobalTransferLimitInPercentage) public withPerm(ADMIN) { | ||
require(isTransferLimitInPercentage, "Transfer limit not set in Percentage"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require 0 <= _newGlobalTransferLimitInPercentage
<= 100
* @param _wallet wallet address | ||
* @param _transferLimitInPercentage transfer limit for the wallet in percentage. Multiple the percentage by 10^16. Eg 22% will be 22*10^16 | ||
*/ | ||
function setTransferLimitInPercentage(address _wallet, uint _transferLimitInPercentage) public withPerm(ADMIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be ideal to make this into a batch function. Pass _wallet and _transferLimitInPercentage as arrays and perform this operation for each item. So on the frontend we can perform these operations in batch.
* @notice removes transfer limit for a wallet | ||
* @param _wallet wallet address | ||
*/ | ||
function removeTransferLimitForWallet(address _wallet) public withPerm(ADMIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, please make into a batch function.
* @param _wallet wallet address | ||
* @param _transferLimit transfer limit for the wallet in tokens | ||
*/ | ||
function setTransferLimitForWallet(address _wallet, uint _transferLimit) public withPerm(ADMIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be ideal to make this into a batch function. Pass _wallet and _transferLimit as arrays and perform this operation for each item. So on the frontend we can perform these operations in batch.
* @notice add an exempt wallet | ||
* @param _wallet exempt wallet address | ||
*/ | ||
function addExemptWallet(address _wallet) public withPerm(ADMIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as other functions. Please make into batch function
* @notice remove an exempt wallet | ||
* @param _wallet exempt wallet address | ||
*/ | ||
function removeExemptWallet(address _wallet) public withPerm(ADMIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as other function. Please make into batch function
|
||
mapping(address => uint) public specialTransferLimits; | ||
|
||
event LogExemptWalletAdded(address _wallet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since some time ago solidity introduced the "emit" keyword, we recently changed all events so they don't start with "log", please remove "Log" prefix from every event.
@subramanianv thanks for submitting the PR! In general terms it looks good! I like how you quickly got the hang out of how our modules have been structured. Left you a few requests for changes, basically making most of the functions so arrays are passed and multiple entries can be added/removed in a single tx. You can check other TM modules to see how we've done it. On top of that, I'd need you to:
Thanks! @satyamakgec could you check if there's anything else I missed? |
@subramanianv Thanks! Now the test coverage is very good. |
@satyamakgec The cleanest approach would be to pause/remove the existing transfer manager and create a new transfer manager with the required config. That was my initial thought. We can also use two different variables to hold the specialtransferlimits as you suggested. lets wait for @pabloruiz55 to jump in. |
…deVolumeRestriction
…k/polymath-core into singleTradeVolumeRestriction
b6f8753
to
8c344e3
Compare
Hey, @subramanianv you can wrap this up using the 2 variables approach. |
@satyamakgec I made into 2 variables. Please review the PR. Thanks :) |
Hey @subramanianv on logical side code is good to go. Please update your branch.
|
…k/polymath-core into singleTradeVolumeRestriction
6457008
to
09015da
Compare
@satyamakgec I made all the requested changes.
|
@satyamakgec there seems to be an issue with tests here, could you check?
|
@satyamakgec Can this PR be closed as the code has been merged into development ? |
Closing this PR as it was merged in #318 |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Closes #258
(Bug fix, feature, docs update, ...)
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?
(What changes might users need to make in their application due to this PR?)
Any Other information: