Skip to content
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

add optimizedReentrancyGuard #443

Merged
merged 3 commits into from Sep 15, 2018

Conversation

Projects
None yet
3 participants
@BrendanChou
Copy link
Contributor

BrendanChou commented Sep 13, 2018

Add this if we release before Open-Zeppelin updates to v1.12

Unfortunately, this change adds a local variable to the modifier, which contributes to the 16-variable stack limit for any function that uses this modifier

@BrendanChou BrendanChou requested a review from AntonioJuliano Sep 13, 2018

*
* Optimized version of the well-known ReentrancyGuard contract
*/
contract OptimizedReentrancyGuard {

This comment has been minimized.

@AntonioJuliano

AntonioJuliano Sep 13, 2018

Collaborator

I think we can just call this ReentrancyGuard

This comment has been minimized.

@AntonioJuliano

AntonioJuliano Sep 13, 2018

Collaborator

and just import our reentrancy guard instead of zeppelin's

@@ -762,8 +762,7 @@ contract BucketLender is
lockedBucket = criticalBucket;
}

uint256 totalOwedToken = 0;
uint256 totalHeldToken = 0;
uint256[2] memory results; // [0] = owedToken, [1] = heldToken

This comment has been minimized.

@AntonioJuliano

AntonioJuliano Sep 13, 2018

Collaborator

why was this changed?

This comment has been minimized.

@BrendanChou

BrendanChou Sep 13, 2018

Author Contributor

So the modifier variable counts toward the 16-var stack limit of functions >:(

This comment has been minimized.

@AntonioJuliano

AntonioJuliano Sep 14, 2018

Collaborator

make this // [0] = totalOwedToken, [1] = totalHeldToken

@@ -40,7 +40,7 @@ import { PositionCustodian } from "../interfaces/PositionCustodian.sol";
* other addresses to close their positions for them.
*/
contract ERC721MarginPosition is
ReentrancyGuard,
OptimizedReentrancyGuard,

This comment has been minimized.

@AntonioJuliano

AntonioJuliano Sep 13, 2018

Collaborator

just double check you got all of the contracts, it's hard to tell on github

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2018

Pull Request Test Coverage Report for Build 7470

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 7442: 0.0%
Covered Lines: 1334
Relevant Lines: 1334

💛 - Coveralls

@BrendanChou BrendanChou force-pushed the bc_optimizedreentrancy branch 2 times, most recently from 65aadab to db9e880 Sep 14, 2018

@BrendanChou BrendanChou force-pushed the bc_optimizedreentrancy branch from db9e880 to f68860f Sep 14, 2018

uint256 private _guardCounter = 1;

modifier nonReentrant() {
// SLOAD

This comment has been minimized.

@AntonioJuliano

AntonioJuliano Sep 14, 2018

Collaborator

this isn't necessary, and I think is confusing

@@ -762,8 +762,7 @@ contract BucketLender is
lockedBucket = criticalBucket;
}

uint256 totalOwedToken = 0;
uint256 totalHeldToken = 0;
uint256[2] memory results; // [0] = owedToken, [1] = heldToken

This comment has been minimized.

@AntonioJuliano

AntonioJuliano Sep 14, 2018

Collaborator

make this // [0] = totalOwedToken, [1] = totalHeldToken

BrendanChou added some commits Sep 14, 2018

@BrendanChou BrendanChou merged commit 5da9356 into master Sep 15, 2018

7 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: checkout_and_install Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: prod_build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@BrendanChou BrendanChou deleted the bc_optimizedreentrancy branch Sep 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.