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

[P2-N13] Remove TODOs #1327

Merged
merged 2 commits into from
Apr 29, 2020
Merged

[P2-N13] Remove TODOs #1327

merged 2 commits into from
Apr 29, 2020

Conversation

ptare
Copy link
Contributor

@ptare ptare commented Apr 29, 2020

I believe these are all either done or we have decided not to do them
(e.g., we're find the implementation of remargin).

Signed-off-by: Prasad Tare prasad.tare@gmail.com

I believe these are all either done or we have decided not to do them
(e.g., we're find the implementation of remargin).

Signed-off-by: Prasad Tare <prasad.tare@gmail.com>
@@ -258,7 +258,6 @@ contract PricelessPositionManager is FeePayer, AdministrateeInterface {
* @dev Might not withdraw the full requested amount in order to account for precision loss.
* @return amountWithdrawn The actual amount of collateral withdrawn.
*/
// TODO: Decide whether to fold this functionality into withdraw() method above.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrice32 We had discussed this, I think its still a decent idea to do this, but maybe not given time constraints.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we decided not to because it would be confusing to have the collateralAmount arg in this function call.

@@ -200,7 +200,6 @@ contract Liquidatable is PricelessPositionManager {
PositionData storage positionToLiquidate = _getPositionData(sponsor);

tokensLiquidated = FixedPoint.min(maxTokensToLiquidate, positionToLiquidate.tokensOutstanding);
// TODO: Limit liquidations from being too small or very close to 100% without being exactly 100%.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one we handle now, +1

@ptare ptare marked this pull request as ready for review April 29, 2020 15:44
@ptare
Copy link
Contributor Author

ptare commented Apr 29, 2020

I think the markdown linting is failing? I think this PR will have to wait for #1328.

@chrismaree
Copy link
Member

I think the markdown linting is failing? I think this PR will have to wait for #1328.

Ye, sorry about that. just caught it now. will be some teathing until our linter works on all markdown. It might actually be worth it to remove it from the CI pipeline so that we don't get blocked like this if .md is pushed that is not compliant.

@ptare
Copy link
Contributor Author

ptare commented Apr 29, 2020

I think the markdown linting is failing? I think this PR will have to wait for #1328.

Ye, sorry about that. just caught it now. will be some teathing until our linter works on all markdown. It might actually be worth it to remove it from the CI pipeline so that we don't get blocked like this if .md is pushed that is not compliant.

Eh no worries.

@ptare ptare added the audit-fix-phase2 Fixes for Phase 2 (EMP) of the OZ audit label Apr 29, 2020
Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -258,7 +258,6 @@ contract PricelessPositionManager is FeePayer, AdministrateeInterface {
* @dev Might not withdraw the full requested amount in order to account for precision loss.
* @return amountWithdrawn The actual amount of collateral withdrawn.
*/
// TODO: Decide whether to fold this functionality into withdraw() method above.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we decided not to because it would be confusing to have the collateralAmount arg in this function call.

@ptare ptare merged commit f42c06d into master Apr 29, 2020
@mrice32 mrice32 changed the title [P02-N13] Remove TODOs [P2-N13] Remove TODOs May 7, 2020
@mrice32 mrice32 deleted the ptare-todo branch May 11, 2020 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-fix-phase2 Fixes for Phase 2 (EMP) of the OZ audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants