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

feature/pro-615/lp-time-based-ranges #3841

Closed
wants to merge 33 commits into from

Conversation

Janislav
Copy link
Contributor

@Janislav Janislav commented Aug 14, 2023

Pull Request

Closes: PRO-615

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

@linear
Copy link

linear bot commented Aug 14, 2023

PRO-609 More control over LP positions lifetime

When creating an order, an LP should have the possibility to specify these parameters:

  • boolean: "Pool-or-Cancel". If this flag is set, the order will only be created if it means that it would not immediately be consumed by an active swap. I.e. this flag indicates the request "make sure that this order is created in the pool, otherwise cancel it"
  • optional block number: "Good-Until". If specified, the order will automatically be burnt in the provided block
  • optional block number: "Good-After". If specified, the order will reserve the necessary funds from the LPs liquidity immediately, but only become active after the provided block

@Janislav Janislav changed the title Feature/pro 609/lp time based ranges feature/pro-609/lp-time-based-ranges Aug 14, 2023
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #3841 (1c58ccd) into main (cad228d) will increase coverage by 0%.
The diff coverage is 80%.

@@          Coverage Diff           @@
##            main   #3841    +/-   ##
======================================
  Coverage     71%     71%            
======================================
  Files        364     364            
  Lines      56218   56476   +258     
  Branches   56218   56476   +258     
======================================
+ Hits       39998   40234   +236     
- Misses     14257   14276    +19     
- Partials    1963    1966     +3     
Files Changed Coverage Δ
api/lib/src/lp.rs 0% <0%> (ø)
state-chain/pallets/cf-pools/src/lib.rs 67% <73%> (+3%) ⬆️
state-chain/pallets/cf-pools/src/tests.rs 97% <98%> (+<1%) ⬆️
state-chain/cf-integration-tests/src/swapping.rs 63% <100%> (ø)
state-chain/pallets/cf-pools/src/benchmarking.rs 100% <100%> (ø)

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Janislav Janislav marked this pull request as ready for review August 24, 2023 15:05
@linear
Copy link

linear bot commented Aug 24, 2023

PRO-615 LP Time-Based Order Validity Range

We want LP's to be able to specify an inclusive range of blocks (possibly timestamps) within which the order is to remain valid.

(Why timestamps? I think this will be useful since blocks are not always a very reliable measure of time).

Imagine an order with a validity range defined by its creation window and valid_until:

  A   B           C
  ↓   ↓           ↓           
|___|___|___|___|___|___|___|___|
      ↑       ↑           ↑
  creation_window     valid_until

There are 3 possibilities:

A: Order added before the creation window → Ok. Funds are reserved, and we defer order submission until the first block of the creation window.

B: Order added inside the creation window → Same as A, except the order is immediately live.

C: Order added after the creation window → Extrinsic Error

Some pseudo-code as a starting point:

struct ValidityWindow<Time: Ord> {
  // Could be Option or we could just use MIN/MAX as defaults.
  open_after: Time,
  open_until: Time,
}

// This is the actual type we use to determine if an order is valid.
// We can extend this later on with Price/Quantity constraints.
struct OrderValidity<Time: Ord> {
   valid_at: ValidityWindow<Time>,
   valid_until: Time,
}

// Allow use of block or timestamp
pub enum BlockOrTimestamp<B, TS> {
  Block(B),
  Timestamp(TS),
}

impl<T: frame_timestamp::Config> Ord for BlockOrTimestamp<
  T::BlockNumber,
  T::Moment,
> {
  // compare blocks, or covert to timestamp if necessary
}

// public interface type for the extrisic argument
// can contain short-cuts to common combinations
// and is then converted into an OrderValidity
pub enum ValidityTypes {
  GoodUntil {
    last: BlockOrTimestamp,
  },
  GoodAfter {
    first: BlockOrTimestamp,
  },
  Custom {
    validity: OrderValidity
  }
}

impl From<ValidityType> for OrderValidity {
  // Convert
}

Other Considerations:

  • We will need a mechanism to withhold the funds. The will need to be blocked and then released again when we activate the order. This could be as simple as subtracting the amount and then adding it back just before we submit the order to the AMM.
  • We should use the on_initialise hook for creation / expiry.

@Janislav Janislav changed the title feature/pro-609/lp-time-based-ranges feature/pro-615/lp-time-based-ranges Aug 24, 2023
Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

I've made the below fixes/changes and also some renames/refactors and added some tests + merge main.

@dandanlen
Copy link
Collaborator

dandanlen commented Aug 28, 2023

@AlastairHolmes I'll leave final approval to you.

A couple of things that might need further consideration:

  • I feel like the user-facing api (lp-api) could be improved - a non-inclusive Range value might not be the most intuitive to use (inclusive might be easier to use, for example if you want to create an order that is valid at exactly block X, then you would currently have to specify a range of X..X+1).
  • It might be worth thinking about what happens if an order is minted, and a expiry/burn is scheduled, but the order is burned/replaced in the interim via another extrinsic. Right now, the scheduled burn would still happen, and would apply to any remaining liquidity in the limit order. This might cause some confusion. Order ids might help avoid any unexpected side effects here.

@dandanlen
Copy link
Collaborator

Just realised #3886 is already open - this will probably have to wait for that...

@Janislav
Copy link
Contributor Author

@AlastairHolmes just wanted to ask what the status is or if this is still on the menu. The last info I had was that you wanted to merge this with LP changes.

@Janislav
Copy link
Contributor Author

I was looking into it but I guess the amount of conflicts is too high that it's probably easier just to re-implement the logic of PRO-609 on top of the new changes which are on main. Are you done with all your changes regarding the pools-pallet @AlastairHolmes? If yes, I would start doing that.

@AlastairHolmes
Copy link
Contributor

I was looking into it but I guess the amount of conflicts is too high that it's probably easier just to re-implement the logic of PRO-609 on top of the new changes which are on main. Are you done with all your changes regarding the pools-pallet @AlastairHolmes? If yes, I would start doing that.

There is one more change to make which I'll do in the next few days. Basically auto collecting which should be easy now.

@AlastairHolmes
Copy link
Contributor

Should this be closed in favour of the new PR? @Janislav

@dandanlen
Copy link
Collaborator

I think so. We can reopen if not.

@dandanlen dandanlen closed this Nov 9, 2023
@ahasna ahasna deleted the feature/PRO-609/lp-time-based-ranges branch May 23, 2024 13:30
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.

3 participants