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

Implement AMM 2.0-light #1092

Merged
merged 81 commits into from
Oct 11, 2023
Merged

Implement AMM 2.0-light #1092

merged 81 commits into from
Oct 11, 2023

Conversation

maltekliemann
Copy link
Contributor

@maltekliemann maltekliemann commented Sep 4, 2023

What does it do?

Implements AMM 2.0-light according to internal design documents.

What important points should reviewers know?

  • Math functions taken from HydraDX because we need exponential for negative numbers and ln for values < 1.
  • https://doc.rust-lang.org/book/ch11-03-test-organization.html used for test organization
  • Going by the view that extrinsic function bodies are for signing and weight logic only and should call into a do_* function, which should contain absolutely no signing or weight logic. Sometimes this results in a double DB reads. A solution is proposed here: Research performance cost of duplicate reads and implement a solution if necessary #1081
  • All test data created using Python playground
  • I found multiple bugs in the to_fixed_decimal conversion: 0.99999999999999999 converts to 0.1 and 2.075187496394219092731299 converts to 0.2751874964. I've found it easier to rewrite the entire function. Because this is a very touchy situation, I suggest taking an extra close look at the new implementation (failed conversions are far worse that rounding errors!). Mistakes could be disastrous.
  • I've tried to make as little changes to other pallets as possible. This sometimes results in awkward code duplication. This will most likely get better when AMM 2.0 is implemented completely.
  • One particular instance where I avoided unncessary work is with destroying markets. LMSR pools are closely tied to the underlying market since they make use of the buy/sell complete set operations. As luck (?) would have it, every extrinsic except exit requires a market to exist. Thus, destroying the underlying market makes the pool defunct. I see no need to make any changes to market destruction.
  • The formatting is god-awful in some places. This is because we're using the nightly version of rustfmt, which is pretty broken. Nothing I can do here except raising issues on rustfmt.
  • I've taken a few shortcuts when extending prediction-markets. In particular, I've deleted the extra_weight stuff because it's Rikiddo-only.
  • I'm double-booking the reserve of the pool in the Pool struct to avoid griefing attacks: Sending funds to the pool can be used to change balances and thus prices. Doesn't seem to be profitable, but certainly very annoying.
  • When creating a pool which uses the native currency as collateral, I'm depositing the existential deposit to avoid problems where a small fee amount < ED is sent to an empty pool account and the entire transaction errors out. This doesn't affect non-native collateral because we're just whitelisting these accounts from dust collection. Native currency doesn't seem to support whitelisting, although I'm getting mixed signals (or I'm just dumb): Add a dust removal whitelist to pallet-balances paritytech/polkadot-sdk#227, https://substrate.stackexchange.com/questions/9824/reaping-vs-dusting-and-how-to-whitelist-an-account.

Is there something left for follow-up PRs?

Follow-up PRs will do the following:

  • Add detailed README and LaTeX docs explaining the formulas used in math.rs.
  • Update zrml-prediction-markets benchmarks and define zrml-neo-swaps benchmarks
  • Add RPC calls
  • Include NeoSwaps in the runtime

What alternative implementations were considered?

None.

Are there relevant PRs or issues?

Strangely enough, this PR doesn't close any issues. Please refer to the design document for more details.

References

Things I've Learned

  • When you abstract functions into traits which the pallet or outside structs implement, make the traits generic over Config. Don't be a hero and try to make the struct independent of Config. Otherwise, type inferrence and error handling is an utter mess.
  • This is the last time I will ever avoid adding magic numbers to tests. A test with 50+ lines is virtually unreadable and completely obfuscates what's actually going on. Better to just provide magic numbers and point reviewers to the prototype which produces the values.
  • It's probably a dumb idea to mix storage reads and non-pallet structs like Pool.

@maltekliemann maltekliemann added the s:in-progress The pull requests is currently being worked on label Sep 4, 2023
@maltekliemann maltekliemann self-assigned this Sep 4, 2023
@maltekliemann maltekliemann added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Sep 4, 2023
@maltekliemann maltekliemann added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Sep 5, 2023
@maltekliemann maltekliemann added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Sep 5, 2023
@maltekliemann maltekliemann added s:review-needed The pull request requires reviews and removed s:review-needed The pull request requires reviews labels Sep 5, 2023
@sea212 sea212 added this to the v0.4.1 milestone Sep 5, 2023
@maltekliemann maltekliemann added s:review-needed The pull request requires reviews and removed s:review-needed The pull request requires reviews labels Sep 29, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 4, 2023

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

@mergify mergify bot added s:revision-needed The pull requests must be revised and removed s:review-needed The pull request requires reviews labels Oct 4, 2023
@maltekliemann maltekliemann added s:review-needed The pull request requires reviews and removed s:revision-needed The pull requests must be revised labels Oct 4, 2023
Chralt98
Chralt98 previously approved these changes Oct 4, 2023
Copy link
Member

@sea212 sea212 left a comment

Choose a reason for hiding this comment

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

LGTM, however I would separate the weight updates into another PR as they bloat the merge commit with unrelated changes.

@mergify
Copy link
Contributor

mergify bot commented Oct 11, 2023

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

@mergify mergify bot added s:revision-needed The pull requests must be revised and removed s:review-needed The pull request requires reviews labels Oct 11, 2023
@maltekliemann maltekliemann added s:review-needed The pull request requires reviews and removed s:revision-needed The pull requests must be revised labels Oct 11, 2023
@maltekliemann maltekliemann merged commit 3d9bbff into main Oct 11, 2023
9 of 12 checks passed
@maltekliemann maltekliemann deleted the mkl-neo-swaps branch October 11, 2023 16:34
@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants