-
Notifications
You must be signed in to change notification settings - Fork 465
Conversation
2d1b7c5
to
074e62c
Compare
bef1983
to
2ab67c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sick! 🤓
Some nits, but looks great overall.
I have an idea that might increase the precision of CD a bit, see the comment in MixinExchangeFees
const decode = () => RevertError.decode(_encoded); | ||
expect(decode).to.be.throw(); | ||
const decoded = RevertError.decode(_encoded, true); | ||
expect(decoded instanceof RawRevertError).to.be.true(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(decoded instanceof RawRevertError).to.be.true(); | |
expect(decoded).to.be.an.instanceof(RawRevertError); |
is a thing I think
protocolFeesThisEpochByPool[poolId] = _feesCollectedThisEpoch.safeAdd(amount); | ||
if (_feesCollectedThisEpoch == 0) { | ||
activePoolsThisEpoch.push(poolId); | ||
if (poolId != 0x0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (poolId != 0x0) { | |
if (poolId != NIL_MAKER_ID) { |
activePoolsThisEpoch.push(poolId); | ||
} | ||
} else { | ||
// No pool associated with `makerAddress`. Refund the fee. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
contracts/staking/test/pools_test.ts
Outdated
@@ -15,6 +15,7 @@ import { StakingWrapper } from './utils/staking_wrapper'; | |||
blockchainTests('Staking Pool Management', env => { | |||
// constants | |||
const ZRX_TOKEN_DECIMALS = new BigNumber(18); | |||
const PPM_ONE = 1e6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's throw these into contracts/staking/test/utils/constants.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could also use DUMMY_TOKEN_DECIMALS
from contracts/test-utils/src/constants.ts
instead of ZRX_TOKEN_DECIMALS
everywhere
contracts/staking/test/pools_test.ts
Outdated
const poolOperator = new PoolOperatorActor(operatorAddress, stakingWrapper); | ||
// create pool | ||
const tx = poolOperator.createStakingPoolAsync(operatorShare); | ||
const expectedPoolId = '0x0000000000000000000000000000000100000000000000000000000000000000'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const expectedPoolId = '0x0000000000000000000000000000000100000000000000000000000000000000'; | |
const expectedPoolId = stakingConstants.INITIAL_POOL_ID; |
export function getNumericalDivergence(a: Numberish, b: Numberish, precision: number = 18): number { | ||
const _toInteger = (n: Numberish) => { | ||
const _n = new BigNumber(n); | ||
const integerDigits = _n.integerValue().sd(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should also be integerValue(BigNumber.ROUND_DOWN)
because otherwise if e.g. n=999.999...
we'd lose a digit of precision
@@ -28,7 +29,7 @@ blockchainTests('End-To-End Simulations', env => { | |||
owner = accounts[0]; | |||
exchange = accounts[1]; | |||
users = accounts.slice(2); | |||
users = [...users, ...users]; // @TODO figure out how to get more addresses from `web3Wrapper` | |||
users = [...users]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch 🎣
/** | ||
* Convert a string, a number, or a BigNumber into a hex string. | ||
*/ | ||
export function toHex(n: string | BigNumber | number): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nifty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a rainy day, we should combine this with the implementation in the ABI Encoder. This implementation is cleaner; the other supports negative values ~ combined, they'd be unstoppable! 🤓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I even use this function. I must have added it in a fever dream. But since you bring it up, I updated it to work with negative values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh that's purdy ~ Ima steal this and put it in the ABI Encoder when you're not looking 🌝
/** | ||
* Create a RevertError instance with optional parameter values. | ||
* Parameters that are left undefined will not be tested in equality checks. | ||
* @param declaration Function-style declaration of the revert (e.g., Error(string message)) | ||
* @param values Optional mapping of parameters to values. | ||
*/ | ||
protected constructor(name: string, declaration?: string, values?: ValueMap) { | ||
protected constructor(name: string, declaration?: string, values?: ValueMap, raw?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param raw
// totalRewards * stakeRatio * e^(alpha * (ln(feeRatio) - ln(stakeRatio))) | ||
|
||
// Compute e^(alpha * (ln(feeRatio) - ln(stakeRatio))) | ||
int256 logFeeRatio = LibFixedMath._ln(feeRatio); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a good handle on how the error of _ln(x)
varies with x
?
Given that we get fewer digits of precision in the ln(x), where x is close to 0
test, I assume it's more numerically stable for x
closer to 1 –– if that's the case, we might be able to squeeze out a bit more precision using the following observation:
ln(F_i / F) - ln(S_i / S))
, ln(F_i / R_i) - ln(F / R)
, -ln(R_i / F_i) - ln(F / R)
, ln(R / F) + ln(F_i / R_i)
, ln(R / F) - ln(R_i / F_i)
are all equivalent.
For given values of F_i, R_i, F, R
, only one of {ln(F_i / R_i) - ln(F / R)
, -ln(R_i / F_i) - ln(F / R)
, ln(R / F) + ln(F_i / R_i)
, ln(R / F) - ln(R_i / F_i)
} will work with _ln
, but it might be more stable than ln(F_i / F) - ln(S_i / S))
.
So we can do some quick comparisons of F_i, R_i, F, R
to get a heuristic of which version will be more precise. This is all a theoretical hunch though, would need testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could also potentially use ln((F_i * R) / (F * R_i))
or -ln((F * R_i) / (F_i * R))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's funny because I originally had your suggestion of ln(x = (F_i * R) / (F * R_i))
but scrapped it since x
would be unbounded. Then I added in the conditional inverse but completely forgot that I could now switch back. I'll make the change and run before and after precision tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆 that error curve is jank
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I played around with adding more high order terms to ln()
to squeeze out more precision. It was pretty negligible any way I sliced it. The huge error spike towards 1
is probably due to the taylor series not having enough terms, so I can look into generating more of those, but I'm not sure if it's worth the extra gas, since I think. in practice, we won't see inputs near 1.0
if the market is well diversified. Also, 14 digits of precision in these cases is probably good enough, I think?
I also did end up settling on the two following cobb-douglas forms, which saves >1k gas and left precision unchanged. I also refactored it somewhat to hopefully reduce code gen.
totalRewards * stakeRatio * e^(alpha * (ln(feeRatio / stakeRatio))) # if feeRatio <= stakeRatio
totalRewards * stakeRatio / e^(alpha * (ln(stakeRatio / feeRatio))) # if feeRatio > stakeRatio
This is because ln(x)
has domain 0 < x <= 1
, exp(x)
has domain x < 0
, and 127-bit fixed-point overflows quite easily when multiplying numbers > ~2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sooo awesome !! Way cleaner and the precision boost in the tests is unreal.
activePoolsThisEpoch.push(poolId); | ||
} | ||
} else { | ||
// No pool associated with `makerAddress`. Refund the fee. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm if we do this then the protocol fee becomes optional. Makers could just opt to not create a pool, allowing all of their takers to bypass the protocol fee.
On the flip side, if we keep this $$ then it creates an incentive for market makers to stake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't reallly sure what was going to happen on the Exchange side with the fees when I did this. I like the idea of just keeping the fee to sweeten the pot. We should all discuss this. This section will have to be totally overhauled for WETH anyhoo.
pure | ||
returns (uint256 ownerRewards) | ||
{ | ||
assert(alphaNumerator <= alphaDenominator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come assert
here instead of require
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it should never happen through regular use. I just went ahead and removed it.
@@ -24,8 +24,7 @@ import "./MixinDeploymentConstants.sol"; | |||
contract MixinConstants is | |||
MixinDeploymentConstants | |||
{ | |||
// TODO: Reevaluate this variable | |||
uint8 constant internal PERCENTAGE_DENOMINATOR = 100; | |||
uint32 constant internal PPM_ONE = 1000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - Renaming to ONE_MILLION
or ONE_PPM
may be more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or PPM_DENOMINATOR
maybe
@@ -1,315 +0,0 @@ | |||
|
|||
|
|||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIP 👏 👏 👏
int256 private constant EXP_MIN_VAL = -int256(0x0000000000000000000000000000001ff0000000000000000000000000000000); | ||
|
||
/// @dev Get one as a fixed-point number. | ||
function _one() internal pure returns (int256 f) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency nit - IIRC we're no longer prefixing library functions with _
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wanted to not, but I figured it was more important to be consistent with the rest of the package, especially with a lot of related PRs in the fray. Aren't we gonna do a convention switch PR at the end anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think once the outstanding PR's are merged all libraries will follow the new convention. But yeah we'll have to a convention upgrade on internal variables so probably fine to do it then.
/** | ||
* Convert a string, a number, or a BigNumber into a hex string. | ||
*/ | ||
export function toHex(n: string | BigNumber | number): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a rainy day, we should combine this with the implementation in the ABI Encoder. This implementation is cleaner; the other supports negative values ~ combined, they'd be unstoppable! 🤓
export type Numberish = BigNumber | string | number; | ||
|
||
/** | ||
* Converts two decimal numbers to integers with `precision` digits, then returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - "Converts two floating point numbers to integers..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk is it fair to call anything that isn't stored in a number
a floating point? I think it's gonna be a lie whatever I put in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yaaa you can disregard this lol. I think I just dropped it in when I realized we need this fn because the decimal is floating but .. yeah.. too nit.
/** | ||
* Asserts that two numbers are equal up to `precision` digits. | ||
*/ | ||
export function assertRoughlyEquals(actual: Numberish, expected: Numberish, precision: number = 18): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this function. Don't you lose information about where the decimals are relative to one another?
Suppose we're comparing 8.99
and 11.5
with precision=3
. This returns abs(899 - 115) = 784
, whereas I would expect something like abs(89 - 115) = 26
, which is a closer representation of the true difference of 2.51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got me! Nice catch.
int256 logFeeRatio = LibFixedMath._ln(feeRatio); | ||
int256 logStakeRatio = LibFixedMath._ln(stakeRatio); | ||
int256 n; | ||
if (logFeeRatio <= logStakeRatio) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - could be more readable to replace this if/else with something like LibFixedMath._subAbs(logFeeRatio, logStakeRatio)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that more readable?? lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk maybe there's a better way but I feel like we can nix the if
smt here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be right. It's the domain issues of the primitives that prohibit certain formulations. That said, I don't think this if
is costing us much gas, though (there's way more in FixedMath). I'll merge for now but keep thinking about it.
} | ||
|
||
/// @dev Returns the multiplication two numbers, reverting on overflow. | ||
function __mul(int256 a, int256 b) private pure returns (int256 c) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you started working on this we've switched to using LibSafeMath
, now that it's a library. Could be good to use it in here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if I wanted to inherit the same error types for SafeMath
in FixedMath
. if I did do it I would feel compelled to also throw SafeMath
errors in places like __add()
, even though it's very different from SafeMath.add()
. Then there's other FixedMathError
types that don't make sense at all for SafeMath
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! love the cobb-douglas and fixed math tests
import { assertRoughlyEquals, getRandomInteger, getRandomPortion, Numberish, toDecimal } from './utils/number_utils'; | ||
|
||
// tslint:disable: no-unnecessary-type-assertion | ||
blockchainTests('Cobb-Douglas', env => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests are slick
0x2bd3386e; | ||
|
||
// solhint-disable func-name-mixedcase | ||
function FixedMathSignedValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional nit: remove the FixedMath
prefix from these functions (LibFixedMathRichErrors.FixedMath...Error
is a bit redundant)
8ee2a07
to
9180b63
Compare
…as()` in `MixinExchangeFees`. `@0x/contracts-staking`: Update `LibFixedMath` to existing naming conventions. `@0x/contracts-staking`: Add `cobbDouglasAlphaNumerator` and `cobbDouglasAlphaDenominator` to `Mixinstorage`. `@0x/contracts-staking`: Add external `setCobbDouglasAlpha()` function to `MixinExchnageFees` `@0x/contracts-staking`: Update `_cobbDouglas()` to use the new `LibFixedMath` voodoo. `@0x/contracts-staking`: In reward calculations, use only delegated stake by pool owner as opposed to both delegated and active stake to compute stake totals.
`@0x/utils`: Add `LibFixedMath` `RevertError` types. `@0x/order-utils`: Add `InvalidCobbDouglasAlphaerror` `RevertError` type.
…()`, and `hexRightPad()` hex utils.
`@0x/utils`: Have Ganache `Error` -> `RevertError` coercion fail if it can't look up the selector.
… the selector is unknown.
…tError` registry.
…ut domains and improve precision. `@0x/contracts-staking`: Add `_invert()` and `_mulDiv()` to `LibFixedMath`. `@0x/contracts-staking`: Update `MixinExchangeFees._cobbDouglas()` to work with `LibFixedMath`. `@0x/contracts-staking`: Add unit and fuzz tests for `_cobbDouglas()` and remaining `LibFixedMath` functions.
…lling `setCobbDouglasAlpha()`.
…evertError` type to `InvalidPoolOperatorShareError`.
`@0x/contracts-staking`: Denominate pool operator shares in parts-per-million. `@0x/contracts-staking`: Update tests for new stake computation and higher precision math. `@0x/contracts-staking`: Add `setCobbDouglasAlpha()` function.
…ors.IndexOutOfBoundsError`.
`@0x/contracts-test-utils`: Add `PPM_DENOMINATOR` and `PPM_100_PERCENT` constants.
…Error`. `@0x/utils`: Use `...is.instanceof()` pattern in `RevertError` tests.
`@0x/contracts-staking`: Remove some unecessary asserts. `@0x/contracts-staking`: Fix some broken test assertions. `@0x/contracts-staking`: Generate better random values in tests. `@0x/contracts-staking`: Rename `PPM_ONE` constant to `PPM_DENOMINATOR`. `@0x/contracts-staking`: Minor solidity code improvements. `@0x/contracts-staking`: Use more constants from `@0x/contracts-test-utils` in tests.
…vertError` types.
…FixedMath` revert errors.
b691b33
to
356660a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so stoked about this update ~~ let's git er merged! (~‾▿‾)~
Description
Flexibler, preciser, efficienter Cobb-Douglas
This PR replaces the math primitives in
LibFeeMath
with a more flexible, efficient, and preciseLibFixedMath
library, based on taylor series approximations of natural logs and exponentials.I previously thought this implementation was more expensive over the original, but these tests say otherwise!
1/x
).The two main files of interest for this are LibFixedMath.sol and MixinExchangeFees.sol.
Add
setCobbDouglasAlpha()
The all-important cobb-douglas alpha factor is now configurable, by the owner, through a
setCobbDouglasAlpha()
function.Correct operator stake calculation
Previously we were counting a pool operator's activated (undelegated) + delegated stake in calculating the total stake of a pool. Now we only use the operator's stake delegated to that pool.
Higher precision pool operator share values
All instances of
operatorShare
are now denominated in parts-per-million (e.g.,500000 == 0.5
). This is up from parts-per-100 (e.g.,50 == 0.5
).The delegated stake weight for computing rewards is also now denominated in PPM.
RevertError Bug Fix
There was a bug/bad feature in the way unknown
RevertError
s were handled when you did anexpect(...).to.revertWith(...)
statement. Essentially, any unknown selectors would turn into anAnyRevertError
type, which always matches successfully with any otherRevertError
type.Now unknown selectors will simply throw an error.
Some minor testing bugs were uncovered (such as new
RevertError
types not being registered) and fixed in this PR.Testing instructions
If you run the tests with
TEST_ALL=1
, you will get a bunch of fuzz tests for the new math primitives and the cobb-douglas function.Tests run with the
UNLIMITED_CONTRACT_SIZE
flag for now because the staking contract is >25k yuge.Types of changes
Checklist:
[WIP]
if necessary.