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

[BUG] Volume #904

Closed
joeykrug opened this issue Jan 26, 2019 · 5 comments
Closed

[BUG] Volume #904

joeykrug opened this issue Jan 26, 2019 · 5 comments
Assignees
Labels
prio/1 High type/bug Something isn't working

Comments

@joeykrug
Copy link
Contributor

Volume has been broken for a few weeks, way too high, dunno why

@joeykrug joeykrug added the type/bug Something isn't working label Jan 26, 2019
@nuevoalex
Copy link
Contributor

nuevoalex commented Jan 26, 2019

In the update-volumetrics function in AN the resulting value is not converted from attoETH to ETH. The volume stored in the DB is in terms of ETH. Since there was no original migration for this value either we can't just trivially modify any getters to convert the value (since it is currently a sum of attoETH values and ETH values).

We need to write a migration script to regenerate this value from the trades table and either:

  • store it as attoETH then convert to ETH in any getter
    or
  • store it as ETH

In the interest of ease for v1 I'd recommend the second approach. For v2 anything we store should be in on-chain / atto units.

@joeykrug
Copy link
Contributor Author

Sure let’s store it as ETH

@ryanberckmans
Copy link
Contributor

ryanberckmans commented Jan 28, 2019

I haven't looked into this, but it seems you guys are reporting that my recent volume fix PR AugurProject/augur-node#777 is handling units improperly.

Note that the fix for this will require a DB reset. [Edit: Alex suggested a fix migration which works]

My PR expects that markets.volume and outcomes.volume are denominated in ETH. Suggest we keep this, so the fix would be converting into ETH when calculating incremental volume.

The fix can probably be entirely isolated to the pure function volumeForTrade.

@nuevoalex if you explain the correct conversion, I'm happy to open a fix PR.

@nuevoalex
Copy link
Contributor

It should be doable without a reset. We can just rebuild the volumes from the trades table in a migration

@adrake33 adrake33 assigned ryanberckmans and unassigned adrake33 Jan 28, 2019
@adrake33 adrake33 added the prio/1 High label Jan 28, 2019
@nuevoalex
Copy link
Contributor

Took a look at the code again and its actually a bit different than attoETH vs ETH. The values being passed to volumeForTrade are actually in terms of ETH at that point, however this means the formula needs alterations since the formula assumes attoETH values when accounting for complete sets.

In particular this portion:

BigNumber.min(p.numCreatorShares, p.numFillerShares).multipliedBy(numTicks));

Will produce a value far too high since numTicks is the value in attoETH of a complete set but this function is returning an ETH value. The equivalent value for this case where the shares are actualy display shares and the desired result is in terms of ETH would be:

BigNumber.min(p.numCreatorShares, p.numFillerShares).multipliedBy(displayRange));

Where the displayRange value comes from the market table by taking its maxPrice - minPrice. In Yes/No and categorical this will always be 1 but for scalars it will vary.

ryanberckmans added a commit to AugurProject/augur-node that referenced this issue Jan 28, 2019
Fixes AugurProject/augur#904 and details in that ticket.
ryanberckmans added a commit to AugurProject/augur-node that referenced this issue Jan 29, 2019
Fixes AugurProject/augur#904 and details in that ticket.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio/1 High type/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants