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

Display error when relay fee is greater than xdai amount #2219

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hexyls
Copy link
Contributor

@hexyls hexyls commented Aug 23, 2021

closes #2189

Displays an error when relay fee is greater than xdai amount for both buys and deposit for xdai markets when connected to the relay.

Also added a warning for when a user is attempting to transaction through the relay with an xDai balance lower than the relay fee.

Testing (all using the relay/omen account):

  • When buying/depositing a tiny amount (0.0001) on a Dai market the following error message is displayed: "Relay fee is greater than buy amount."
  • Withdraw all Dai from your Omen account, when buying/depositing/selling/creating the following message is displayed: "Relay fee is greater than your DAI balance, please top up your Omen account."
  • Switch to Rinkeby and double check that these messages are not displayed in any markets

@hexyls hexyls marked this pull request as draft August 23, 2021 06:57
@hexyls hexyls marked this pull request as ready for review August 24, 2021 06:05
@hexyls hexyls requested review from Mi-Lan and pimato August 24, 2021 06:05
@pimato
Copy link
Contributor

pimato commented Aug 24, 2021

Testing (all using the relay/omen account):

  • When buying/depositing a tiny amount (0.0001) on a Dai market the following error message is displayed: "Relay fee is greater than buy amount."
  • Withdraw all Dai from your Omen account, when buying/depositing/selling/creating the following message is displayed: "Relay fee is greater than your DAI balance, please top up your Omen account."
  • Switch to Rinkeby and double check that these messages are not displayed in any markets

@pimato
Copy link
Contributor

pimato commented Aug 24, 2021

I would prefer the usage of our error message below the inputfield like this:
Bildschirmfoto 2021-08-24 um 16 45 43

Figma:
Design Specification

@hexyls
Copy link
Contributor Author

hexyls commented Aug 25, 2021

@pimato I moved the relay fee error under inputs but I'm unable to access that design spec atm 🤔

@pimato
Copy link
Contributor

pimato commented Aug 25, 2021

@pimato I moved the relay fee error under inputs but I'm unable to access that design spec atm 🤔

Sorry @hexyls , fixed!

@pimato
Copy link
Contributor

pimato commented Aug 25, 2021

I would add a new error message for xDai tight user who have not deposit Dai into the Omen account. Right now we are just saying Insufficient balance if the user has no balance for the specific asset. I think before that we should show the message Insufficient Dai in your Omen Account for deposit/withdraw/buy/sell if the Omen Account is not funded:

Bildschirmfoto 2021-08-25 um 10 52 10

Priority of error message for xDai tight:

  1. If Omen Account has no DAI show Insufficient Dai in your Omen Account
  2. If Omen Account Dai balance > 0 we do the classic error message path like insufficient balance if the user inputs a higher amount he has available and so on.

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.

@hexyls Great job with service! Code looks good added some stuff wondering what you think.

Comment on lines +49 to +51
return () => {
status.active = false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering just out of learning purposes why are you returning this status.active=false. And what does it do?

Copy link
Contributor Author

@hexyls hexyls Sep 2, 2021

Choose a reason for hiding this comment

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

this prevents against race conditions when requesting data, e.g. imagine we start requesting data while the user is using the relay and the user switches accounts mid request - it is possible that the state could be updated with data we don't want once the relayService.getInfo() request finishes.

The line you commented will make sure status.active is set to false if the hook is updated, so in our scenario above once relayService.getInfo() is finished we check status.active, if it is true that means the hook has not updated since the request was made and it is safe to update the state.

@@ -11,7 +11,7 @@ const Wrapper = styled.div<{ margin?: boolean }>`
border: ${({ theme }) => theme.borders.borderLineDisabled};
align-content: center;
padding: 4px 20px;
margin-bottom: ${props => (!props.margin ? '20px' : '0')};
margin-bottom: ${props => (props.margin ? '20px' : '0')};
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't find figma designs but seems that because you flipped evaluation in some cases where it was previously set we have double margins...Like if value inputted in negative on market_buy component. Or this is how its supposed to be I couldn't find figma @pimato ?
Screenshot 2021-08-26 at 14 58 13

Copy link
Contributor

Choose a reason for hiding this comment

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

@pimato Also seems that color on this error message when value is negative is different then the error message on form...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

filled this back to !props.margin and changed up the alert color 👍

formField: any
symbol: any
shouldDisplayMaxButton?: boolean
onClickMaxButton?: () => void
style?: CSSProperties | undefined
}

const FieldWrapper = styled.div`
const FieldWrapper = styled.div<{ error?: boolean }>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this into style(TYPE.BodyRegular) and remove couple of prperties like font-size,font-weight,line-height

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmmm will leave this one, styled(TYPE.bodyRegular) doesn't appear to apply font styling in the same way using the> input selector does

Comment on lines +289 to +296
: relayFeeGreaterThanBalance
? 'Insufficient Dai in your Omen Account'
: maybeCollateralBalance.isZero() && funding.gt(maybeCollateralBalance)
? `Insufficient balance`
: funding.gt(maybeCollateralBalance)
? `Value must be less than or equal to ${collateralBalanceFormatted} ${collateral.symbol}`
: relayFeeGreaterThanAmount
? 'Relay fee is greater than buy amount'
Copy link
Contributor

Choose a reason for hiding this comment

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

@hexyls Also one more consideration. I was looking through the codebase now and seems that in this pr and throught the codebase in components(marketBuy,marketSell,fundingAndFee and MarketPoolLiqudity) we have almost exactly the same amount error evaluations. Wondering does it make sense to make separate component or service dedicated to it? So we don't mess up in future if we have to add some new error handling. What do you think?

Copy link
Contributor

@Mi-Lan Mi-Lan Aug 26, 2021

Choose a reason for hiding this comment

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

if you consider this makes sense to do we can even move it also into separate issue...

@Mi-Lan
Copy link
Contributor

Mi-Lan commented Aug 26, 2021

Screen.Recording.2021-08-26.at.15.57.42.mov

@hexyls Also seems that message gets stuck and I can't even buy with proper amount inputted..After I triggered insufficient dai in your omen account

@hexyls
Copy link
Contributor Author

hexyls commented Sep 2, 2021

@Mi-Lan for that last one where the error stays - do you know how to reproduce it? I managed to get it to show up once but haven't seen it since 🤔

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.

Not able to purchase outcome with 0.001 Collatearal
3 participants