-
Notifications
You must be signed in to change notification settings - Fork 0
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
test: unit tests for BFactory.sol #2
Conversation
src/contracts/test/TToken.sol
Outdated
@@ -127,7 +127,7 @@ contract TToken { | |||
function transferFrom(address src, address dst, uint amt) external returns (bool) { | |||
require(msg.sender == src || amt <= _allowance[src][msg.sender], "ERR_BTOKEN_BAD_CALLER"); | |||
_move(src, dst, amt); | |||
if (msg.sender != src && _allowance[src][msg.sender] != uint256(-1)) { | |||
if (msg.sender != src && _allowance[src][msg.sender] != uint256(int(-1))) { |
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.
use type(uint256).max
instead 🙏
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.
looking good!
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 think this is sth that may improve on the way forward, but we could have standarized names for testing, such as:
test_Set_Owner
test_Set_IsBPool
test_Emit_Logs
test_Returns_Pool
and so, in that way, we can then filter them out for example to be run individually, or by topic
test/unit/BFactory.t.sol
Outdated
* @notice Test that a valid pool is present on the mapping | ||
*/ | ||
function test_validPoolIsBPool() public { | ||
BPool _pool = bFactory.newBPool(); |
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.
can we avoid deploying a new pool but instead overwriting the storage with cheatcodes? 🙏
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! good job! 💪
No description provided.