Skip to content

Conversation

@bravepooh
Copy link
Contributor

  1. Clean Contracts and migrations folders.
  2. enable MaxPerToken in allowTokens.sol

}
function setMaxPerToken(address token, uint256 _maxAmount) external onlyOwner {
require(_maxAmount <= maxTokensAllowed, "AllowTokens: Max Tokens should be equal or smaller than Max Tokens");
require(minAllowedToken[token] <= _maxAmount, "AllowTokens: maxAllowedToken[] should be equal or bigger than minAllowedToken[]");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also validate this in setFeeAndMinPerToken (if maxAllowdToken[token] != 0)

@@ -0,0 +1,10 @@
const MultiSigWallet = artifacts.require("MultiSigWallet");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how the migrations are used, but is it right to edit the old migration instead of creating a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup of migrations folder and contracts folder

assert.equal(result, true);

result = await this.allowTokens.isValidTokenTransfer(this.token.address, web3.utils.toWei('2'), 0, true);
assert.equal(result, true);
Copy link
Contributor

@koirikivi koirikivi May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add one test here, setMaxPerToken(this.token.address, 0, {from: manage}) and then check that the maximum is unset (ie. 3 is valid again)

Copy link
Contributor

@koirikivi koirikivi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some small suggestions. I think the only thing that 100% should be done is to validate that the minimum is not set bigger than maximum in setFeeAndMinPerToken but generally it's ok, no bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants