Skip to content

Add CloseMarket function to redeem rent from permissioned markets #191

Open
Henry-E wants to merge 26 commits into
project-serum:masterfrom
Henry-E:CloseMarkets
Open

Add CloseMarket function to redeem rent from permissioned markets #191
Henry-E wants to merge 26 commits into
project-serum:masterfrom
Henry-E:CloseMarkets

Conversation

@Henry-E

@Henry-E Henry-E commented Nov 3, 2021

Copy link
Copy Markdown

Currently a placeholder pull request for adding a close market function.

Still TODO

Questions for @armaniferrante

  • How to check that the events, bid and asks queues are empty before removing rent from them?
  • Do we need a market closed flag to markets so that it's clear the market is shutdown?

Things I will add

  • Add CloseMarkets to the permissioned markets proxy example
  • Add a test to the permissioned markets javascript tests

Plus any other code review and additional checks that need to be added to the function.

The pull request is mainly based on existing examples from how CloseOpenOrders and Prune functions were implemented.

@Henry-E

Henry-E commented Dec 2, 2021

Copy link
Copy Markdown
Author

Added an anchor test directory that initializes a market and shuts it down again, then checks that the sol is retrieved.

It's not ready for merging yet though. There needs to be more discussion of what checks are needed before the rent is retrieved. Right now though the core functions are there and there's a working test. So fitting in any requested changes ought to be straightforward. We will see though!

Next up

  • Investigate what variables are available on the queue structs to tell whether they are truly empty, similar to the free_slot_bits variable on the open orders struct

Ok, it looks like it's possible to load up request queue and event queue then check their headers to ensure count = 0. For bid and ask queue it might be possible to access their SlabHeaders to check the leaf count, although not sure yet.

Update: Added checks that load the headers for all four queue structs and checks that their counts are empty.

Comment thread dex/src/state.rs Outdated
Comment thread dex/src/state.rs Outdated
Comment thread dex/tests/close-markets/.gitignore Outdated
Comment thread dex/tests/close-markets/Cargo.lock Outdated
Comment thread dex/tests/close-markets/package-lock.json Outdated
Comment thread dex/tests/close-markets/programs/close-markets/src/lib.rs
@armaniferrante

Copy link
Copy Markdown
Contributor

How to check that the events, bid and asks queues are empty before removing rent from them?

What the code currently does is correct. The event queue can check the header count and the bid/asks slabs can check the leaf_count.

@Henry-E

Henry-E commented Dec 14, 2021

Copy link
Copy Markdown
Author

Ok, added the disabled flag. Going to start adding more comprehensive testing. It looks like the permissioned markets set of tests are failing because they're not using solana 1.9.0

@Henry-E Henry-E left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Finished add close market and a more full set of tests

Comment thread dex/src/state.rs
Comment thread dex/src/instruction.rs Outdated
Comment thread dex/src/state.rs Outdated
Comment thread dex/src/state.rs Outdated
Comment thread dex/tests/close-markets/tests/close-markets.ts Outdated
Comment thread dex/tests/close-markets/tests/close-markets.ts Outdated
@Henry-E

Henry-E commented Dec 15, 2021

Copy link
Copy Markdown
Author

@armaniferrante Ran a full set of tests on the new close market functionality.

close_markets doesn't allow markets to be closed while

  • orders are in the bid or ask queue
  • there are uncranked events

And even more importantly

  • Open orders accounts can be settled and closed after the markets has been closed

@armaniferrante armaniferrante left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to add the tests to travis CI

Comment thread dex/src/state.rs
**bids_acc.lamports.borrow_mut() = 0;
**asks_acc.lamports.borrow_mut() = 0;

market.account_flags = market.account_flags | (AccountFlag::Disabled as u64);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should use Closed instead of Disabled.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Serum already has the functionality for Disabled built in from the ability to manually shutdown. I didn't find anything similar for Closed

@Henry-E Henry-E Jan 5, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

https://github.com/project-serum/serum-dex/blob/master/dex/src/state.rs#L1884 - Here's where new order already checks for whether a market is disabled. Is this functionality different from closing? It's possible I haven't fully understood the full extent of what the disable flag affects

Comment thread dex/tests/close-markets/programs/close-markets/src/lib.rs
Comment thread dex/tests/close-markets/programs/close-markets/src/lib.rs
Comment thread dex/tests/close-markets/programs/close-markets/src/lib.rs
Comment thread dex/tests/close-markets/programs/close-markets/src/lib.rs
Comment thread dex/tests/close-markets/test-ledger/validator.log Outdated
Comment thread dex/tests/close-markets/test-ledger/faucet-keypair.json Outdated
@Henry-E

Henry-E commented Jan 7, 2022

Copy link
Copy Markdown
Author

Travis CI tests added.

Arguably we still haven't come to a good conclusion on the use of disabled. Serum was already design such that markets could be disabled by some authority in extreme scenarios so we make use of that existing functionality. Only the new_order_v3 function has an explicit market_enabled() check, all the other functions can still be called but will fail if they rely on the existence of any of the queue accounts.

I wanted to keep the actual changes made to the dex as minimal as possible. However, if it's necessary we can re-use the market_enabled check in any of the functions that shouldn't be possible to call post-disabling, e.g. cancel_order, consume events. Any functions involving open orders accounts, e.g. close open orders or settle, should be still be allowed to be called post-disabling, so we wouldn't have to make any changes to them.

@0xBooler

0xBooler commented Feb 8, 2022

Copy link
Copy Markdown

bump sers @Henry-E @armaniferrante pls help 🥏

@wdstudionft

Copy link
Copy Markdown

up @Henry-E @armaniferrante
I think this legit solution

@wdstudionft

Copy link
Copy Markdown

bump

@bigdefidev

bigdefidev commented Jul 1, 2022

Copy link
Copy Markdown

what's the status of this? Any forthcoming plans to try to include this feature soon?

@gaburipeach

Copy link
Copy Markdown

bump

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.

7 participants