-
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
Removes activeStakeRequestCount from OSTComposer #775
Removes activeStakeRequestCount from OSTComposer #775
Conversation
- Updates test cases accordingly. - Adds `npm run update` script to ease updating submodules and npm packages after switching branches. Fixes: OpenST#772
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.
👍 This looks good.
I think the following things can be done on this ticket due to this change.
-
We don't need safe math now.
import "openzeppelin-solidity/contracts/math/SafeMath.sol"; -
We should add the check for the existence of staker proxy contract when we do accept stake request.
StakerProxy stakerProxy = stakerProxies[stakeRequest.staker];
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 👍
Please see one inline comment.
- Asserts explicitly an extence of staker proxy during acceptStakeRequest. - Fixes lint issues in acceptStakeRequest unit test. - Removes SafeMath as its not needed after removing activeStakeRequestCount.
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 🚀
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.
Almost done 👍
Just one last thing to add,
A unit test case corresponding to the following condition
require(
address(stakerProxy) != address(0),
"StakerProxy address is null."
);
Adds a test case to test a failure of OSTComposer::acceptStakeRequest in case if a corresponding staker proxy does not exist.
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
npm run update
script to ease updating submodules and npmpackages after switching branches.
Fixes: #772