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

Compound integration with Omen Markets. Archived in favor of https://github.com/protofire/omen-exchange/pull/1570 #1466

Closed
wants to merge 59 commits into from

Conversation

pRoy24
Copy link
Contributor

@pRoy24 pRoy24 commented Dec 17, 2020

Fixed the previous round of comments.
Added badge as defined in the mockup.
Refactored to make it work with ETH as the base and cETH as the underlying collateral.
Tested on Rinkeby with ETH -> cETH and dai -> cDAI pairs.

Please note that you need to run yarn install since this feature uses Big.js version 6.0.0

@pRoy24 pRoy24 changed the title Gitcoin bounty submission. https://gitcoin.co/issue/dxdaohackathon/omen-exchange/1/100024191 Gitcoin bounty submission. https://gitcoin.co/issue/dxdaohackathon/omen-exchange/1/100024191 by proy24 Dec 17, 2020
@pRoy24 pRoy24 marked this pull request as draft January 1, 2021 11:33
@pRoy24 pRoy24 changed the title Gitcoin bounty submission. https://gitcoin.co/issue/dxdaohackathon/omen-exchange/1/100024191 by proy24 Compound integration with Omen Markets. Gitcoin bounty dxdaohackathon/omen-exchange/1/100024191 Jan 4, 2021
@pRoy24 pRoy24 marked this pull request as ready for review January 5, 2021 04:28
@pimato
Copy link
Contributor

pimato commented Jan 15, 2021

just played around with it on Rinkeby and I am getting an error after clicking on circle button to enable it:
Bildschirmfoto 2021-01-15 um 16 35 19

Steps to reproduce:

  1. Connect to Rinkeby
  2. Create Market
  3. Configure Market:

Bildschirmfoto 2021-01-15 um 16 37 04

4) Click Continue 5) Click enable Compound circle button 6) BUG: App crashes

@pRoy24
Copy link
Contributor Author

pRoy24 commented Jan 16, 2021

just played around with it on Rinkeby and I am getting an error after clicking on circle button to enable it:
Bildschirmfoto 2021-01-15 um 16 35 19

Steps to reproduce:

  1. Connect to Rinkeby
  2. Create Market
  3. Configure Market:
Bildschirmfoto 2021-01-15 um 16 37 04
  1. Click Continue 5) Click enable Compound circle button 6) BUG: App crashes

You need to run Yarn install since it uses Big.js version 6.0.0 for the prec function

@pimato
Copy link
Contributor

pimato commented Jan 18, 2021

Hey @pRoy24 , once I enable compound the pool number is jumping around:
Bildschirmvideo aufnehmen 2021-01-18 um 13 11 50

@pimato
Copy link
Contributor

pimato commented Jan 18, 2021

Every market info component here has a svg in 24x24. The compound svg you included is 16x16:
Bildschirmfoto 2021-01-18 um 13 17 55

please use this svg:
comp.svg.zip

@pimato
Copy link
Contributor

pimato commented Jan 18, 2021

Please remove the styling if the circle button is active, currently if you hover over it you see the border is popping up:
Bildschirmfoto 2021-01-18 um 13 13 11

@pimato
Copy link
Contributor

pimato commented Jan 18, 2021

If you hover over the compound info the icon is turning blue, which is not according my design specification. Currently:
Bildschirmfoto 2021-01-18 um 13 21 37

Should:
Bildschirmfoto 2021-01-18 um 13 23 39

onHover color: #00897B

@pimato
Copy link
Contributor

pimato commented Jan 18, 2021

Just looking at the price of the outcomes here:
Bildschirmfoto 2021-01-18 um 13 27 24

I believe those prices are in cDAI and not in DAI right?
I think this is also the case with the shares!

Are we able to calculate all of this to DAI?

@pimato
Copy link
Contributor

pimato commented Jan 18, 2021

Looks like after I done a swap, the form is not resetting which is our default.

Doing a swap:
Bildschirmfoto 2021-01-18 um 13 31 05

after the swap is done, the inputfield is still filled with the number 2 and I see order details:
Bildschirmfoto 2021-01-18 um 13 31 54

It should reset the order form as we do on default.

@pimato
Copy link
Contributor

pimato commented Jan 19, 2021

Please remove the styling if the circle button is active, currently if you hover over it you see the border is popping up:
Bildschirmfoto 2021-01-18 um 13 13 11

it looks like now the the border got removed when holding on-click:
Bildschirmfoto 2021-01-19 um 09 27 24

@pimato
Copy link
Contributor

pimato commented Jan 19, 2021

The compound icon is still not correct, currently:
Bildschirmfoto 2021-01-19 um 10 14 33

should:
Bildschirmfoto 2021-01-19 um 10 13 40

did you actually replaced the svg which I provided?
The currently used svg has viewBox="0 0 16 16" but should actually have viewBox="0 0 24 24"

@pimato
Copy link
Contributor

pimato commented Jan 19, 2021

If you hover over the compound info the icon is turning blue, which is not according my design specification. Currently:
Bildschirmfoto 2021-01-18 um 13 21 37

Should:
Bildschirmfoto 2021-01-18 um 13 23 39

onHover color: #00897B

you changed the normal color to #00897B, but you actually should use #00897B as the hover color for the icon and the text.

Copy link
Contributor

@pimato pimato left a comment

Choose a reason for hiding this comment

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

see comments.

Copy link
Contributor

@Mi-Lan Mi-Lan left a comment

Choose a reason for hiding this comment

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

Small changes...

Comment on lines 72 to 78
const ServiceTextWrapper = styled.div`
width: 90%;
`

const ServiceCheckWrapper = styled.div`
width: 10%;
color: transparent;
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think defining static values to width here is neccesary. I would suggest just adding
justify-content: space-between; to the parent element that is 100% and omitting width for these two values. The reason for this is because 10% is slightly more than 40px and creates this unnecessary margin...
Screenshot 2021-01-20 at 23 28 34

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 261 to -263
cdai: {
symbol: 'cDAI',
decimals: 18,
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2021-01-20 at 23 31 04
Would be good to have consistent letter formatting for both currencies. To make them both capital or with first letter Uppercase (which is according to design...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes sense refactored to include the change.

@pRoy24 pRoy24 mentioned this pull request Jan 22, 2021
19 tasks
@pRoy24 pRoy24 changed the title Compound integration with Omen Markets. Gitcoin bounty dxdaohackathon/omen-exchange/1/100024191 Archived in favor of https://github.com/protofire/omen-exchange/pull/1570 Compound integration with Omen Markets. Gitcoin bounty dxdaohackathon/omen-exchange/1/100024191 Jan 22, 2021
@pRoy24 pRoy24 changed the title Archived in favor of https://github.com/protofire/omen-exchange/pull/1570 Compound integration with Omen Markets. Gitcoin bounty dxdaohackathon/omen-exchange/1/100024191 Compound integration with Omen Markets. Archived in favor of https://github.com/protofire/omen-exchange/pull/1570 Jan 22, 2021
@pRoy24 pRoy24 closed this Jan 22, 2021
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.

5 participants