Implement market creator incentives

[sea212]

What does it do?

• Converts the market creator fee from u8 to Perbill and allows specification of it during market creation within configurable boundaries
• Supplies a migration to update market structures to convert the market creator fee type to Perbill
• Respects the sum of all fees during pool creation (currently LP fee and market creator fee)
• On trades, does:
  • Pay fees to the market creator in base asset if it is contained within the token pair
  • Tries to swap the fee denoted in outcome asset to the base asset and then pays it to the market creator otherwise. If not possible, pays the fees in outcome asset

What important points should reviewers know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues?

closes #739

References

• Proposal: https://zeitgeist.subsquare.io/post/14

[mergify]

This pull request is now in conflicts. Could you fix it @sea212? 🙏

[maltekliemann]

Looks fine to me so far.

[mergify]

This pull request is now in conflicts. Could you fix it @sea212? 🙏

[maltekliemann]

Can't find any serious flaws in the trading logic. There are three points of criticism:

• It seems like you're running into the problem of EDs, similar to the issues I've had with neo-swaps: If the market creator has no funds left, then small fee payments fail. Specifically, the following tests all fail:

  #[test_case(BASE_ASSET, ASSET_B; "base_asset_in")]
  #[test_case(ASSET_B, BASE_ASSET; "base_asset_out")]
  #[test_case(ASSET_B, ASSET_C; "no_base_asset")]
  fn swap_exact_amount_out_creator_fee_swaps_correct_amount_out(
      asset_in: Asset<MarketId>,
      asset_out: Asset<MarketId>,
  ) {
      ExtBuilder::default().build().execute_with(|| {
          let creator_fee = Perbill::from_percent(1);
          let swap_fee = 0;
          assert_ok!(set_creator_fee(DEFAULT_MARKET_ID, creator_fee));
          create_initial_pool_with_funds_for_alice(ScoringRule::CPMM, Some(swap_fee), true);
          assert_ok!(Currencies::withdraw(ASSET_A, &ALICE, Currencies::free_balance(ASSET_A, &ALICE)));
          assert_ok!(Currencies::withdraw(ASSET_B, &ALICE, Currencies::free_balance(ASSET_B, &ALICE)));
          assert_ok!(Currencies::withdraw(ASSET_C, &ALICE, Currencies::free_balance(ASSET_C, &ALICE)));
          assert_ok!(Currencies::withdraw(ASSET_D, &ALICE, Currencies::free_balance(ASSET_D, &ALICE)));
  
          let alice_balance_out_before = Currencies::free_balance(asset_out, &ALICE);
          let asset_amount_out = zeitgeist_primitives::constants::CENT / 10;
  
          assert_ok!(Swaps::swap_exact_amount_out(
              alice_signed(),
              DEFAULT_POOL_ID,
              asset_in,
              Some(u128::MAX),
              asset_out,
              asset_amount_out,
              None,
          ));
  
          let alice_balance_out_after = Currencies::free_balance(asset_out, &ALICE);
          assert_eq!(alice_balance_out_after - alice_balance_out_before, asset_amount_out);
      });
  }

  This is sort-of a problem if people use small creator fees and then reap their account. Basically, they will brick all trades on the market. This is purely a griefing vector, but I think we should fix this by either burning the fee or leaving it in whichever account it currently is (either LPs or the informant receive it).
• I believe there's a bit of an informational issue. It seems that swap fees are taken first, and then creator fees are taken from the remainder. Which means that if creator fees are 1%, then the creator actually doesn't receive 1% of the gross trade amount, but rather only 0.99%. No big deal. I think we should not try to change this. We can improve upon this with the new AMM.
• The benchmarks now depend on how fees are handled. I believe the worst-case is now the one where fees taken in outcomes are swapped for the base asset.

As stated in the comments, I would also feel more comfortable if the tests contained more checks that verify that everyone's balances are the way they're supposed to be.

[sea212]

I believe there's a bit of an informational issue. It seems that swap fees are taken first, and then creator fees are taken from the remainder. Which means that if creator fees are 1%, then the creator actually doesn't receive 1% of the gross trade amount, but rather only 0.99%. No big deal. I think we should not try to change this. We can improve upon this with the new AMM.

Yes, we talked about this. To avoid unnecessary swaps, fees are taken that way:

1. IN: base, out: OUTCOME -> take fees before trade
2. IN: OUTCOME, out: base -> take fees after trade to avoid unnecessary swap
3. IN: OUTCOME, OUT: OUTCOME -> take fees after trade, requires swap.

Unfortunately the current design does not allow to provide a proper fee handler where one could specify when how many fees are taken. Instead, LP fees are always left in the pool after the trade.

A solution to that problem would be to swap the IN OUTCOME for the base asset in 2. and 3. before the trade happens.

[maltekliemann]

I believe there's a bit of an informational issue. It seems that swap fees are taken first, and then creator fees are taken from the remainder. Which means that if creator fees are 1%, then the creator actually doesn't receive 1% of the gross trade amount, but rather only 0.99%. No big deal. I think we should not try to change this. We can improve upon this with the new AMM.

Yes, we talked about this. To avoid unnecessary swaps, fees are taken that way:

1. IN: base, out: OUTCOME -> take fees before trade
2. IN: OUTCOME, out: base -> take fees after trade to avoid unnecessary swap
3. IN: OUTCOME, OUT: OUTCOME -> take fees after trade, requires swap.

Unfortunately the current design does not allow to provide a proper fee handler where one could specify when how many fees are taken. Instead, LP fees are always left in the pool after the trade.

A solution to that problem would be to swap the IN OUTCOME for the base asset in 2. and 3. before the trade happens.

I'm fine with keeping this the way it is. Just making sure we're on the same page.

[sea212]

The benchmarks now depend on how fees are handled. I believe the worst-case is now the one where fees taken in outcomes are swapped for the base asset.

f483b80

[sea212]

It seems like you're running into the problem of EDs, similar to the issues I've had with neo-swaps: If the market creator has no funds left, then small fee payments fail

9101013
8d9f099