Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

Fix issue #2503

Closed
wants to merge 4 commits into from
Closed

Fix issue #2503

wants to merge 4 commits into from

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Feb 25, 2022

Summary

<<if there's an issue>>Fixes #issueNumber

High-level description of what your changes are accomplishing

Add screenshots if applicable. Images are nice :)

To Test

  1. <> Open the page about
  • <<What to expect?>> Verify it contains about information...
  • Checkbox Style list of things a QA person could verify, i.e.
  • Should display Text Input our storybook
  • Input should not accept Numbers
  1. <> ...

Background

Optional: Give background information for changes you've made, that might be difficult to explain via comments

@anxolin anxolin changed the base branch from develop to master February 25, 2022 19:35
@github-actions
Copy link
Contributor

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@alfetopito
Copy link
Contributor

I'll document the explanation of what's happening in this issue.

Without code changes just adding a few debugging entries, we get to this:

Screen Shot 2022-02-25 at 14 52 41

The first part is logging stuff from https://github.com/gnosis/cowswap/blob/d42aabe6f0/src/custom/state/claim/hooks/index.ts#L970-L975

This is where the total max investment amount in investment currency is calculated.
This is where the weird amount is coming from

The second part is logging stuff from

return { vCowAmount: price.quote(amount), investmentCost: amount }

This is where the source for the missing atom is coming, causing amounts to be 99.99% claimed, but not quite 100%

@alfetopito
Copy link
Contributor

alfetopito commented Feb 26, 2022

Why is the amount not precise in the first place? (the weird 25000000000000050000000000000)

Without diving deep into how JSBI/BigNumber and etc handle the amounts, my assumption would be something funny with its math.

From the contract, we get that 1 vCOW = 150000 atoms of USDC == 0.15 USDC
Inverting the price we get 1 USDC = 1000000000000000000/150000 == 6666666666666 (18 decimals from vCOW - 6 decimals from USDC) == 6.666666667 vCOWs

Which is not a precise price, but that's how we represent it in the UI and also how we work with internally

Also, let's take a look at the amounts.

The raw claim data for this case we get is 166666666666667000000000 in vCOW atoms == 166,666.666666667 vCOW

Applying the price to it, we get the weird representation of 25000000000000050000000000000/1000000000000000000 == 25000000000 atoms of USDC == 25000 USDC

Leandro added 2 commits February 25, 2022 17:59
@alfetopito
Copy link
Contributor

Here's regarding the last missing amount on the way back.

Screen Shot 2022-02-25 at 18 10 49

As can be seen for the second claim, the original claim amount is 166666666666667000000000, while the amount we get by applying the price (0.15 USDC per vCOW) we get 166666666666666666666666

const amount = CurrencyAmount.fromRawAmount(investCurrency, investedAmount)
return { vCowAmount: price.invert().quote(amount), investmentCost: amount }
Copy link
Contributor

Choose a reason for hiding this comment

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

but what is the difference between inverting above and now here?

Copy link
Contributor

Choose a reason for hiding this comment

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

is the math found in the method used on line975 fixing this? if so, damn, that's sketchy af for us to be using this math lib...

@@ -62,7 +62,7 @@ import useIsMounted from 'hooks/useIsMounted'
import { ChainId } from '@uniswap/sdk'
import { ClaimInfo } from 'state/claim/reducer'

const CLAIMS_REPO_BRANCH = 'gip-13'
Copy link
Contributor

Choose a reason for hiding this comment

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

careful here about the package (just note)

@anxolin
Copy link
Contributor Author

anxolin commented Mar 17, 2022

@alfetopito let me know if we close this one or u want it open

@alfetopito
Copy link
Contributor

I was supposed to do a post-morten after finishing with the uni-merge.
Keeping it open will be better to remind something must still be done :)

@anxolin anxolin added the On_hold There's unresolved blockers. label Mar 24, 2022
@anxolin anxolin closed this Apr 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
On_hold There's unresolved blockers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants