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

#51 free liquidity -- Draft PR #66

Closed
wants to merge 42 commits into from
Closed

#51 free liquidity -- Draft PR #66

wants to merge 42 commits into from

Conversation

blewater
Copy link
Contributor

Please note, changesets from previous open PRs have crept in.

A. Draft PR for soliciting feedback on completed 1..4 issue items.

  1. Make the gas associated with all market messages identical, so that MsgCancelLimitOrder potentially carries same cost as MsgCancelReplaceLimitOrder.
  2. New orders that add liquidity should receive a gas rebate proportional to how much of the order was filled already (0% fill means 100% rebate):
    if addToBook {
  3. Canceled orders always pay full gas.
  4. Replaced orders are only eligible for the gas rebate in 2. if they are replacing an order older than, say, 5 minutes. This requires Add creation timestamp to orders #34 to work.

B. This PR does not address issue section: Additional considerations.

Suggestions below

It must not be possible to exploit the rebate mechanism in a way where it becomes cheaper (or even free) to remove liquidity, i.e. crafting an order to get almost filled but adding a small amount to the book should not result in a rebate.
It must not be possible to spam the chain with useless transactions. Only behaviour that is net adding liquidity

Half baked ideas

  1. Make Liquidity Fee a minimal dust amount i.e. 250 resulting in free single transactions, but costly for sending a large number of transactions. A known practice.
  2. Do not rebate more than x number of free liquid orders per account at any time. Not a great solution in isolation.
  3. Do not rebate more than x number of duplicate instruments orders per account at any time.
  4. Do not rebate any duplicate instrument order that does not offer qty/pricing spread differential above a threshold i.e. the moving average of the last x booked instruments orders + 15%.

C. Three Market parameters have been added

  • May leave them like that (beginblock)
  • Add them to genesis
  • Const them (what if we change them?)
message TxParams {
  // default fee for a market transaction.
  uint64 trx_fee = 1;

  // Valid liquidity adding orders are free(0) or set to this minimal fee
  uint64 liquid_trx_fee = 2;

  // Minutes interval for eligible replacing transactions to receive a rebate.
  // For a rebate to apply, the replacing transaction should occur these minutes
  // after the signer's original trx.
  int64 liquidity_rebate_minutes_span = 3;
}

@blewater blewater requested review from haasted and mdyring May 18, 2021 15:50
@blewater blewater linked an issue May 18, 2021 that may be closed by this pull request
x/buyback/abci.go Outdated Show resolved Hide resolved
x/market/keeper/keeper.go Outdated Show resolved Hide resolved
x/market/keeper/keeper.go Outdated Show resolved Hide resolved
x/market/types/market.pb.go Outdated Show resolved Hide resolved
x/market/keeper/querier.go Outdated Show resolved Hide resolved
x/market/keeper/keeper.go Show resolved Hide resolved
x/market/keeper/keeper.go Show resolved Hide resolved
@blewater
Copy link
Contributor Author

blewater commented Jun 2, 2021

Freezing this feature while a better SDK or an alternative solution is found.

Accomplished in this work are predictable gas costs for market orders within the market module.
However, before entering the market order, RunTx() spends gas outside within the Ante handlers chain, which in the DeliverTx tests they ramp up to ~55000. These can be zeroed out with an extra ante handler for the market module but at the severe disadvantage of exposing a security vulnerability of gas-free transactions when the market module panics.

A solution explored for mitigating the security vulnerability is adding a BaseApp custom panic handler, combined with the handler having access to the transaction gas meter for charging a gas fee which sadly it does not. As a future consideration, we could open an issue with the Cosmos SDK team to add gas meter access to panic-handlers applicable to this and other business functionality.

@blewater blewater closed this Jun 2, 2021
@blewater
Copy link
Contributor Author

blewater commented Jun 7, 2021

There is a completed Cosmos SDK PR that meets the needs of this functionality, and it was stalled because of a lack of an accompanied ADR. We could pick up the pieces for this and contribute the ADR and facilitate the re-opening of the stalled PR to complete this add gas refund handler to refund the remaining gas fee at the end of transaction #8993.

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.

Market: Adding liquidity should be free
2 participants