-
Notifications
You must be signed in to change notification settings - Fork 33
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
refactor: using bitwise operations to save gas #104 #165
Conversation
@0xneves Hey, ready for review. |
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.
Thank you a lot @blackbeard002 !! Just a few minor improvements and I will submit the PR, I think the hardest one its the NatSpec, since it need good communication xD
LFG
contracts/SwapFactory.sol
Outdated
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.
Could you provide a NatSpec description for this function explaining how it works?
Since the explanation should stay in the Interface contract, the implementation should be used the reference just like in the rest of the contract e.g.:
/**
* @dev See {ISwaplace-getSwap}.
*/
contracts/Swaplace.sol
Outdated
if (swap.expiry < block.timestamp) revert InvalidExpiry(swap.expiry); | ||
if (expiry < block.timestamp) revert InvalidExpiry(expiry); | ||
|
||
expiry = 0; |
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 line is unnecessary as the 0
can be directly inputted in line 66 in the expiry variable of packData
contracts/interfaces/ISwap.sol
Outdated
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 file was commited using tabWidth = 4, but the correct is tabWidth = 2.
The following should follow pattern and should be changed from:
* - `config` holds two values 'allowed' and 'expiry',packed using bitwise.
* 'allowed' address to accept the Swap.
* 'expiry' date of the Swap.
To:
* - `config` represents two packed values: 'allowed' for the allowed address
* to accept the swap and 'expiry' for the expiration date of the swap.
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.
Could you provide a NatSpec description for this function explaining how it works
test/TestSwapFactory.test.ts
Outdated
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.
Avoid using underscores to name variables in the test file, since there is a conflict, the best method would be to rename the variables that don't resemble the contracts, for instance:
- Change
_expiry
intotimestamp
- Keep the config expiry as expiry, since its the same inside the contract implementation
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.
Changing the expiry
into configExpiry
is also a go, but the first recommendation suits more the legibility.
test/TestSwaplace.test.ts
Outdated
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.
Same recommendation as the TestSwapFactory.test.ts
Hey @0xneves I've made the changes that you requested. Ready for review. |
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'm approving although I had to brute force some fixes to speed up development
The test/TestSwapFactory.test.ts
I'll manually fix in another issue as it's lacking legibility
closes #104