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

Issue 159 add position caps #174

Merged
merged 4 commits into from
Dec 29, 2023
Merged

Issue 159 add position caps #174

merged 4 commits into from
Dec 29, 2023

Conversation

markuspluna
Copy link
Contributor

In accordance with issue 159 added a max_positions variable that is controllable by the pool admin. This allows pool admins to prevent user's from opening too many positions resulting in resource limits being exceeded when someone attempts to liquidate them.

@markuspluna markuspluna requested a review from mootz12 December 21, 2023 23:18
@markuspluna markuspluna changed the base branch from main to issue_160_test_auction_underflow December 21, 2023 23:18
Copy link
Contributor

@mootz12 mootz12 left a comment

Choose a reason for hiding this comment

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

It's also worth considering if we want to add logic to protect users while reducing their overall postion size.

Consider - max positions (4) user positions (4)

Pool updates the max positions to (2) to fix a stuck liquidation or something

user goes to repay a borrowed asset (or user positions is staged for liquidation?), and they can't. Worse off, they could be withdrawing a dust position?

Comment on lines 232 to 233
// Fetch the current max posoitions
///
Copy link
Contributor

Choose a reason for hiding this comment

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

fix comment

Comment on lines 241 to 244
/// Set a new admin
///
/// ### Arguments
/// * `max_positions` - The max positions for the pool
Copy link
Contributor

Choose a reason for hiding this comment

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

fix comment

Comment on lines 283 to 292
fn set_max_positions(e: Env, max: u32) {
storage::extend_instance(&e);
let admin = storage::get_admin(&e);
admin.require_auth();

storage::set_max_positions(&e, &max);

e.events()
.publish((Symbol::new(&e, "set_max_positions"), admin), max);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should likely be included with pool_init_meta, right? Like backstopTakeRate, it feels like a vital piece of setup required for the pool to function and should be set on initialization.

In the same vein, it feels appropriate to include this with update_pool or rename that function to set_backstop_rate to avoid confusion.

Having one function that sets multiple pieces of pool configuration doesn't seem problematic to me, but I don't have a strong opinion either way.

Base automatically changed from issue_160_test_auction_underflow to main December 27, 2023 18:28
@mootz12 mootz12 self-requested a review December 29, 2023 15:27
Copy link
Contributor

@mootz12 mootz12 left a comment

Choose a reason for hiding this comment

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

Everything looks good, just a small optimization I noticed.

Going to go ahead and push a commit up to implement it.

Comment on lines 283 to 284
// Verify max positions haven't been exceeded
pool.require_under_max(e, from_state.positions.clone(), request.request_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can likely get away with only checking this after all requests have been processed.

We know how many positions they started with, so if it increases, just need to make sure its at most the maximum.

@mootz12 mootz12 merged commit 043e98f into main Dec 29, 2023
3 checks passed
@mootz12 mootz12 deleted the issue_159_add_position_caps branch December 29, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants