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

PoE mixing function #205

Merged
merged 18 commits into from
Oct 10, 2021
Merged

PoE mixing function #205

merged 18 commits into from
Oct 10, 2021

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Oct 8, 2021

Closes #9.

TODO:

  • More tests.
  • Add a validate() method.
  • Benchmarks.
  • More functions:
    • Algebraic sigmoid. (Done in AlgebraicSigmoid)
    • Sigmoid-like with exponent p = 1/2. (Done in SigmoidSqrt)
    • Piece-wise (saturating) impl.

@maurolacy maurolacy added the poe Proof of Engagement label Oct 8, 2021
@maurolacy maurolacy added this to the v0.4.1 milestone Oct 8, 2021
@maurolacy maurolacy self-assigned this Oct 8, 2021
ethanfrey
ethanfrey previously approved these changes Oct 8, 2021
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

I am happy to merge this as is.
It sets up the framework for user-configurable curves quite well.
It also proves the potential of using decimal power and exponents (very nice).

I mention a few minor cleanup items that can be addressed in a future PR.
Other points that would be interesting to me would be:

  • Implement some variant of the your comment using x / sqrt(x^2 + 1) as the sigmoid curve (where x is some function of stake and engagement)
  • Some gas benchmarks of these various approaches. We just need wasm gas benchmarks, so no need to run multiple runs (criteria) but rather one test case that runs in a vm and records gas. This would be similar to the integration tests in cosmwasm/contracts

Note: I cannot import cosmwasm-vm on my Mac m1, so it would be nice to make that whole dependency optional under some feature flag.

contracts/tg4-mixer/src/error.rs Show resolved Hide resolved
contracts/tg4-mixer/src/functions.rs Show resolved Hide resolved
contracts/tg4-mixer/src/msg.rs Show resolved Hide resolved
contracts/tg4-mixer/src/msg.rs Outdated Show resolved Hide resolved
contracts/tg4-mixer/src/functions.rs Show resolved Hide resolved
contracts/tg4-mixer/src/functions.rs Show resolved Hide resolved
contracts/tg4-mixer/src/functions.rs Show resolved Hide resolved
contracts/tg4-mixer/src/functions.rs Show resolved Hide resolved
assert_eq!(err, ContractError::ComputationOverflow("powd"));

// Precise limit
let very_big = 32_313_447;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... this is a good limit to document.
It would be good to test with some reasonable numbers.

Note that stake is measured in utgd. We could expect maybe 5 million TGD (5% of tokens) as an extreme case.
This would be 5_000_000_000_000 stake (much bigger than the limit).

On the other hand, we will have maybe 1 million engagement total, so let's say 5_000 engagement as a high value.

Basically, it would be an interesting tests to see the maximum stake that we can not overflow with 5_000 engagement.

Copy link
Contributor Author

@maurolacy maurolacy Oct 8, 2021

Choose a reason for hiding this comment

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

It's just this number squared, divided by 5_000, i.e. 208_831_771_404. Which is not enough, according to your comment above. We can perhaps convert / down-scale to TGD (or mTGD) first, and then it will suffice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, just add a FIXME comment there and I will think of this

Copy link
Contributor Author

@maurolacy maurolacy Oct 8, 2021

Choose a reason for hiding this comment

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

Bad news, the number is exactly the same (32_313_447) because what is overflowing is powd. That said, powd is not very robust / smart, because a number raised to an exponent smaller than 1 will always be smaller than the number(!).

So, this looks like a flaw / limitation in powd's implementation. I'll open an issue in rust_decimal, to see what can be done about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@maurolacy maurolacy Oct 9, 2021

Choose a reason for hiding this comment

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

This actually has a couple simple solutions:

  • Use an exponent p = 1/2 instead of p = 0.68. This can be implemented using Decimal::sqrt() (which doesn't suffer from this problem); or even, integer_sqrt(). Done in the SigmoidSqrt impl.
  • stake = 32_313_447 and engagement = 5_000 already produces a saturating reward (1_000). So, we can add a check for numbers above these values, and return max_r directly. This also saves some cycles, and extends the range up to u63::MAX for both factors. This can be done, but requires some care to detect when a particular parametrization saturates.

contracts/tg4-mixer/src/functions.rs Show resolved Hide resolved
@ethanfrey ethanfrey modified the milestones: v0.4.1, v0.5.0 Oct 8, 2021
@maurolacy
Copy link
Contributor Author

maurolacy commented Oct 8, 2021

  • Implement some variant of the your comment using x / sqrt(x^2 + 1) as the sigmoid curve (where x is some function of stake and engagement)

I plan to do this next. (done)

  • Some gas benchmarks of these various approaches. We just need wasm gas benchmarks, so no need to run multiple runs (criteria) but rather one test case that runs in a vm and records gas. This would be similar to the integration tests in cosmwasm/contracts.

Will do.

Note: I cannot import cosmwasm-vm on my Mac m1, so it would be nice to make that whole dependency optional under some feature flag.

OK.

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Thanks for adding all those curves and tests

@ethanfrey ethanfrey merged commit 61d5939 into main Oct 10, 2021
@ethanfrey ethanfrey deleted the 9-poe-mixing-function branch October 10, 2021 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
poe Proof of Engagement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tg4-mixer: Configure "mixing function" for PoE
2 participants