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

Integrate Bid Transactions into actual flow #160

Merged
merged 26 commits into from
Mar 30, 2022

Conversation

fegbert
Copy link
Contributor

@fegbert fegbert commented Mar 23, 2022

closes #125

@fegbert
Copy link
Contributor Author

fegbert commented Mar 23, 2022

Couple of questions on some values:

Auction Minimum Bid: What actually is this value / where does it come from?

Allow access to DAI: Is this also setting isWalletAuthorised or is this doing something else?

AllowanceAmount: What's this used for / Do I use even this or is it for the wallet popup?

@valiafetisov
Copy link
Contributor

Auction Minimum Bid: What actually is this value / where does it come from?

Good question, I think I forgot to fetch this value. Will open a new PR to add this value to each auction

Allow access to DAI: Is this also setting isWalletAuthorised or is this doing something else?

This is actually not a binary value as initially thought of, but a BigNumber. In the auth store it's called allowanceAmount. The value indicates how much DAI the user can deposit to VAT. So it should be not less then the minimum to deposit

@valiafetisov
Copy link
Contributor

valiafetisov commented Mar 28, 2022

So the proposal for this PR is to:

  • remove AccessDaiBlock from the transaction panel (since it's only needed if there is not enough DAI in the wallet)
  • simplify DepositBlock by only leaving Manage DAI in VAT/Enough DAI is available button
  • attach Manage DAI in VAT to the actual modal (when Add Wallet Popup Component #149 is merged)
  • update stories
  • provide correct loading states

MakerDAO@2x (1)

@fegbert
Copy link
Contributor Author

fegbert commented Mar 28, 2022

simplify DepositBlock by only leaving Manage DAI in VAT/Enough DAI is available button

Do I keep Minimum to deposit and the explanation text like proposed in the mock below or really only the button?

@valiafetisov
Copy link
Contributor

Do I keep Minimum to deposit and the explanation text like proposed in the mock below or really only the button?

Sorry for the confusion. You can keep the Minimum to deposit and other values in the table too (balance and balance in VAT), requested change only affects the buttons. I've updated the mock above to reflect this.

@fegbert
Copy link
Contributor Author

fegbert commented Mar 28, 2022

Any ideas already on how to mock the wallet popup / depositing stuff in the fake container and storybook? 🙂

@valiafetisov
Copy link
Contributor

Any ideas already on how to mock the wallet popup / depositing stuff in the fake container and storybook?

Clicking it can just add enough DAI to participate, so you can basically skip the popup logic.

@fegbert fegbert marked this pull request as ready for review March 29, 2022 08:29
@fegbert
Copy link
Contributor Author

fegbert commented Mar 29, 2022

As discussed in today's meeting, the bidding logic is still missing and the button currently just executes the same way as a swap transaction.

@valiafetisov valiafetisov mentioned this pull request Mar 29, 2022
Copy link
Contributor

@valiafetisov valiafetisov left a comment

Choose a reason for hiding this comment

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

Everything else looks good, the only thing left is to merge #163 and attach it to the button. If you're blocked, you can already do that (git merge bidding-with-dai-logic into your branch)

valiafetisov
valiafetisov previously approved these changes Mar 29, 2022
@LukSteib LukSteib self-requested a review March 29, 2022 14:39
Copy link
Contributor

@LukSteib LukSteib left a comment

Choose a reason for hiding this comment

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

  • The minimum to deposit amount in DepositBlock should update properly with each price drop (currently only after browser refresh)
  • The finished auction state is not displayed properly. Bid button is still clickable (throwing an error) and the etherscan link is not displayed (see loom 1 below)

1
https://www.loom.com/share/cfaf4ba6abb54986a811015f64ff187d

@valiafetisov
Copy link
Contributor

Bid button is still clickable (throwing an error) and the etherscan link is not displayed (see loom 1 below)

This should be probably done in #163
I removed transaction link because since we introduce partial bidding, there is a possibility that the user will bid part of the auction and we don't yet have logic to handle multiply transactions per auction 🙁

@LukSteib
Copy link
Contributor

because since we introduce partial bidding, there is a possibility that the user will bid part of the auction and we don't yet have logic to handle multiply transactions per auction

Makes sense. Fine to remove it then for this flow for now.

@valiafetisov valiafetisov merged commit 8604b16 into main Mar 30, 2022
@valiafetisov valiafetisov deleted the integrate-bid-transaction branch March 30, 2022 10:09
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.

Integrate BidTransaction into the flow
3 participants