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

[ECO-2618] Finalize emojicoin arena core logic design #478

Open
wants to merge 58 commits into
base: main
Choose a base branch
from
Open

Conversation

alnoki
Copy link
Member

@alnoki alnoki commented Dec 20, 2024

Description

This PR finalizes the Emojicoin Arena Move prototype, adding a README with a
comprehensive design overview.

Per extensive review, it adopts a lightweight data model that provides only the
minimal amount of information to required to enable comprehensive indexing,
as discussed in detail in the README.

Logic updates

  1. Add assorted inner abstractions to ease the review process.
  2. Eliminate SmartTable structs since they can be a DoS vector.
  3. Remove fields, events that are not required for lightweight data model.
  4. Require that if a user has locked in since their most recent to an empty
    escrow, they must select lock in on subsequent top-off operations.
  5. Allow user to elect lock-in even if they do not ultimately get matched, to
    prevent unexpected abortions.
  6. Add exchange rate calculations.
  7. Reset available rewards for a melee when it is cranked out.
  8. Use integers instead of aggregators since exchange rate calculations require
    queries against both markets and thus inhibit parallelism within a melee
    in the general case.
  9. Refactor type checking logic since exchange rate calculations do type checks.
  10. Add assorted abstractions, helpers.
  11. Add view functions and return unpackers for all core structs.
  12. Add test-only fixturing to enable comprehensive state/event mocking.

Test code

  1. Add assorted mocking assertion functions.
  2. Add additional test coin factories for init_module happy path tests.
  3. Add init_module happy path tests for assorted pseudo-random seeds.
  4. Add admin function happy path and expected failure tests

Housekeeping

  1. Use term active instead of current to indicate a melee that hasn't ended.
  2. Update doc comments to link to actual structs, e.g. "Escrow" instead of
    "escrow".
  3. Take out error codes used for admin-level guard rails that have since been
    removed.
  4. Use compound assignment operators now that they are supported by movefmt
  5. Change default melee time to 20 hours for new linear interpolation matching.

Testing

From in src/move/emojicoin_arena:

git ls-files | entr -c sh -c " \
    aptos move compile --dev --move-2  &&
    aptos move fmt \
"

Checklist

  • Did you check all checkboxes from the linked Linear task?

Copy link

vercel bot commented Dec 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
emojicoin-dot-fun ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 1:15am
emojicoin-dot-fun-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2025 1:15am

@alnoki alnoki requested review from CRBl69 and xbtmatt December 20, 2024 23:45
@alnoki alnoki marked this pull request as draft December 21, 2024 16:34
@alnoki alnoki marked this pull request as ready for review December 22, 2024 21:46
@alnoki alnoki requested review from CRBl69 and xbtmatt and removed request for CRBl69 and xbtmatt December 22, 2024 21:50
@alnoki alnoki requested review from CRBl69 and xbtmatt and removed request for CRBl69 and xbtmatt December 30, 2024 23:56
Copy link
Collaborator

@CRBl69 CRBl69 left a comment

Choose a reason for hiding this comment

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

Look mostly good 👍

Just a couple of DX changes.

src/move/emojicoin_arena/sources/emojicoin_arena.move Outdated Show resolved Hide resolved
src/move/emojicoin_arena/sources/emojicoin_arena.move Outdated Show resolved Hide resolved
src/move/emojicoin_arena/sources/emojicoin_arena.move Outdated Show resolved Hide resolved
src/move/emojicoin_arena/sources/emojicoin_arena.move Outdated Show resolved Hide resolved
src/move/emojicoin_arena/sources/emojicoin_arena.move Outdated Show resolved Hide resolved
Comment on lines 267 to 277
/// Based on proceeds when exiting, otherwise based on holdings in escrow.
struct ProfitAndLoss has copy, drop, store {
/// Emojicoins effective value, converted to octas at current exchange rate.
octas_value: u128,
/// Unrealized gain if `octas_value` is greater than `Escrow.octas_entered`.
octas_gain: u128,
/// Unrealized loss if `octas_value` is less than `Escrow.octas_entered`.
octas_loss: u128,
/// Ratio of `octas_value` to `Escrow.octas_entered`, as a Q64.
octas_growth_q64: u128
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we have octas_initial_value (octas value at time of enter) and octas_current_value (currently octas_value) ?

This would mean that $\rm{octas\_gain} = \rm{octas\_current\_value} - \rm{octas\_initial\_value}$, so no need to include the three other fields (octas_{gain,loss,growth_q64}).

I think it will also be useful to have the initial amount in the frontend.

Copy link
Member Author

@alnoki alnoki Jan 3, 2025

Choose a reason for hiding this comment

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

Since users can enter multiple times, the initial_value descriptor isn't totally accurate, however I do understand the rationale for using a difference here and for including the reference value octas_entered for the frontend

Mainly the octas_gain field is because that is used for determining the top gains in top_exits fields

Copy link
Member Author

Choose a reason for hiding this comment

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

@CRBl69 per the recent lightweight data model we discussed, this can all be done offchain. Note that I even added an example in the new README about indexing PnL and how all the information is there

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.

2 participants