-
Notifications
You must be signed in to change notification settings - Fork 31
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
Reward calculation clean up for zero gas. #527
Reward calculation clean up for zero gas. #527
Conversation
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.
Nice 🎆
A test would be nice. You could use a "spy" or similar to see when mint is called and when it is not called.
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.
Nice👍
I just have a suggestion see inline.
Tests are missing.
Also please document your finding of the points mentioned in the #495
…ttps://github.com/gulshanvasnani/mosaic-contracts into gulshan/gh_495_reward_calculation_cleanup_zero_gas
…o reward to facilitator
…ttps://github.com/gulshanvasnani/mosaic-contracts into gulshan/gh_495_reward_calculation_cleanup_zero_gas
…ttps://github.com/gulshanvasnani/mosaic-contracts into gulshan/gh_495_reward_calculation_cleanup_zero_gas
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.
Nice 🙌
Few comments inline.
Also, it seems you are testing progressMint
not progressMintInternal
. Please add remaining test cases for progressMint
…gh_495_reward_calculation_cleanup_zero_gas
…ttps://github.com/gulshanvasnani/mosaic-contracts into gulshan/gh_495_reward_calculation_cleanup_zero_gas
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 added some comments in-line. I don't have the required knowledge to approve this PR. Summoning @deepesh-kn and @sarvesh-ost
contracts/test/MockUtilityToken.sol
Outdated
* @notice This is for testing purpose. | ||
* | ||
*/ | ||
contract MockUtilityToken is UtilityToken { |
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.
Maybe call this a spy and not a mock?
/cc @deepesh-kn @sarvesh-ost
Should we clean up our test doubles naming in a separate ticket?
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.
In this case, it should not be MockUtilityToken
. For such a case, we use TestUtilityToken
.
Mock... is to completely mock a contract. Test.. is to Test the existing contract and has some way to set data (for testing)
* | ||
* @notice This is used for testing purpose. | ||
*/ | ||
contract TestEIP20CoGateway is EIP20CoGateway { |
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.
How do we currently differentiate between Test...
and Mock...
?
/cc @deepesh-kn @sarvesh-ost
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.
Mock... is to completely mock a contract.
Test.. is to test existing contract function, and has some functions to set data (for testing)
symbol = "OST", | ||
name = "Simple Token", | ||
decimals = 18, | ||
helper; |
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.
The length of these declarations looks unhealthy. Did we mess up with the architecture?
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.
Is it fine to keep these declarations as of now ? Do you have any strong opinion against it.
This makes writing the tests easy.
…ttps://github.com/gulshanvasnani/mosaic-contracts into gulshan/gh_495_reward_calculation_cleanup_zero_gas
…eward_calculation_cleanup_zero_gas
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.
🍷 nice.
Almost complete. Few tests are missing.
contracts/test/MockUtilityToken.sol
Outdated
* @notice This is for testing purpose. | ||
* | ||
*/ | ||
contract MockUtilityToken is UtilityToken { |
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.
In this case, it should not be MockUtilityToken
. For such a case, we use TestUtilityToken
.
Mock... is to completely mock a contract. Test.. is to Test the existing contract and has some way to set data (for testing)
contracts/test/MockUtilityToken.sol
Outdated
) | ||
public | ||
UtilityToken(_symbol, _name, _decimals, _valueToken) | ||
{ |
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.
{ }
…eward_calculation_cleanup_zero_gas
…ttps://github.com/gulshanvasnani/mosaic-contracts into gulshan/gh_495_reward_calculation_cleanup_zero_gas
…and moved the setting up of message in before each
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.
LGTM 🛫
@sarvesh-ost, please to go through the tests once.
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.
Nice, almost done. 🙌 🥇
Some feedbacks inline.
There should be supporting unit test cases for reward calculation, as progressMint
calculates reward as well.
…eward_calculation_cleanup_zero_gas
… statements, passing false for _proofProgress param for progressMintInternal
…ttps://github.com/gulshanvasnani/mosaic-contracts into gulshan/gh_495_reward_calculation_cleanup_zero_gas
…eward_calculation_cleanup_zero_gas
c57443c
to
b167ec9
Compare
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.
LGTM 🎉 💯
Modified progressMintInternal method of EIP20CoGateway to not transfer reward to facilitator if reward is 0.
Unit test-cases to verify mint was called are also added.
Fixes #495
Fixes #433