Reward calculation clean up for zero gas. #527
Conversation
There was a problem hiding this comment.
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
…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.
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.
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.
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.
How do we currently differentiate between Test... and Mock...?
/cc @deepesh-kn @sarvesh-ost
There was a problem hiding this comment.
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.
The length of these declarations looks unhealthy. Did we mess up with the architecture?
There was a problem hiding this comment.
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
contracts/test/MockUtilityToken.sol
Outdated
| * @notice This is for testing purpose. | ||
| * | ||
| */ | ||
| contract MockUtilityToken is UtilityToken { |
There was a problem hiding this comment.
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) | ||
| { |
…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
…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
gulshanvasnani
force-pushed
the
gulshan/gh_495_reward_calculation_cleanup_zero_gas
branch
from
c57443c
to
b167ec9
Compare
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