Add multi-callee support to the frontend #498

aomafarag · 2022-10-13T14:35:01Z

Closes #496.

Checklist:

issue number linked above after pound (#)
- replace "Closes " with "Contributes to" or other if this PR does not close the issue
issue checkboxes are all addressed
manually checked my feature / not applicable
wrote tests / not applicable
attached screenshots / not applicable

LukSteib

Storybook lgtm ✔️
Checks are failing

LukSteib

Component approved

valiafetisov · 2022-10-17T07:42:24Z

Looking at the screenshot:

Please make sure that prices shown in the table are ordered from the best (on top) to the worst (on the bottom) to the Unknown (on the very bottom) on the frontend
(It might be just storybook, and might not need to be fixed) Unknown callee can never be selected (as it seems to be now, since Market Difference is Unknown, and both other prices have Select in front of them)
(It might be just storybook, and might not need to be fixed) In case Market Difference is Unknown, profits and and Estimated Profitability Time will be unknown too

Why issue checkboxes are all addressed is not checked? Is there something outstanding?

aomafarag · 2022-10-19T08:30:34Z

Please make sure that prices shown in the table are ordered from the best (on top) to the worst (on the bottom) to the Unknown (on the very bottom) on the frontend

Done.

(It might be just storybook, and might not need to be fixed) Unknown callee can never be selected (as it seems to be now, since Market Difference is Unknown, and both other prices have Select in front of them)

(It might be just storybook, and might not need to be fixed) In case Market Difference is Unknown, profits and and Estimated Profitability Time will be unknown too

I think it's related to storybook and the pending refactoring of the core, because these values are assigned directly and randomly through helper functions.

Why issue checkboxes are all addressed is not checked? Is there something outstanding?

Nope. Just checked them all.

frontend/components/auction/collateral/MarketPriceSelection.vue

core/src/types.ts

frontend/components/auction/collateral/MarketPriceSelection.vue

valiafetisov · 2022-10-19T11:41:42Z

frontend/components/auction/collateral/CollateralAuctionSwapTransactionTable.vue

-                <span v-else class="opacity-50">Unknown</span>
-            </div>
-        </div>
+        <MarketPriceSelection :auction-transaction="auctionTransaction" :market-id.sync="marketId" />


As you raised in the issue and I answered: the values underneath this row is dependent on the selected marketId. They all now taken from the marketData[marketId] object: so that switching marketId displays new values

Exactly. But the calculation of the market difference, profitability, etc. happens in the core. So, shouldn't that be implemented there by using the marketId that is sent to the store?

OK, nvm. For some reason, I've just noticed the comment you made last week 😅.

LukSteib

Maybe storybook related but to re-iterate on point @valiafetisov mentioned in prev comment

when expanding component for the first time both selectable callees have "Select" and the uni v3 which is actually selected is not marked accordingly ("Selected")
Why is the uni v3 callee the default selection (see screenshot below) if the 1inch callee has a better marketUnitPrice?

aomafarag · 2022-10-21T07:56:43Z

when expanding component for the first time both selectable callees have "Select" and the uni v3 which is actually selected is not marked accordingly ("Selected")

It's not marked because as long as the user hasn't selected any specific callee the suggestedMarketId is displayed, which can also change, as long as the user hasn't made a selection. Once the user makes a selection, it'll be fixed throughout.

For reference:

Expect that suggestedMarketId might change after the component render –> meaning that

in case no selection was made by the user, selected market might change by itself

in case a selection was made by the user, it should stay fixed even if suggestedMarketId changes

— from issue description

Why is the uni v3 callee the default selection (see screenshot below) if the 1inch callee has a better marketUnitPrice?

I picked an arbitrary value for storybook, but I can implement a function that picks the best unit price there as well.

LukSteib · 2022-10-21T08:41:12Z

Thanks for clarification! Makes sense and no need to write a dedicated function for storybook.

aomafarag · 2022-11-04T18:36:04Z

Currently, I get this error in the browser console whenever I select a callee, which is self-explanatory and reflects the issue we are having:

[Vue warn]: Avoid mutating a prop directly since the value will be overwritten whenever the parent component re-renders. Instead, use a data or computed property based on the prop's value. Prop being mutated: "marketId"

found in

---> <CollateralAuctionSwapTransactionTable> at components/auction/collateral/CollateralAuctionSwapTransactionTable.vue
       <SwapTransaction> at components/auction/collateral/CollateralAuctionSwapTransaction.vue
         <SplitLayout> at components/layout/SplitLayout.vue
           <CollateralAuctionFlow> at components/auction/collateral/CollateralAuctionFlow.vue
             <AuctionsContainer> at containers/AuctionsContainer.vue
               <Pages/collateral.vue> at pages/collateral.vue
                 <Nuxt>
                   <Layouts/default.vue> at layouts/default.vue
                     <Root> [vue.runtime.esm.js:619](webpack:///node_modules/vue/dist/vue.runtime.esm.js?2b0e)

~~@valiafetisov, I can't seem to figure out how to avoid mutating the prop directly and syncing it at the same time.~~

@LukSteib, regarding the problem you mentioned with the route. The collateral configuration of wstETH on Curve V3 has an empty route array, i.e. it gets swapped directly into DAI without passing by ETH.

LukSteib · 2022-11-04T18:39:59Z

@LukSteib, regarding the problem you mentioned with the route. The collateral configuration of wstETH on Curve V3 has an empty route array, i.e. it gets swapped directly into DAI without passing by ETH.

I think the problem is not related to WSTETH since same problem occurs for a normal ETH auction. However, this is details that we might fix in a follow up issue.

Currently, I get this error in the browser console whenever I select a callee, which is self-explanatory and reflects the issue we are having:

Get the same error which prevents switching the callee via the UI. This problem however is blocking and I won't approve this PR as I don't want to merge and release a broken component.

aomafarag · 2022-11-05T19:13:58Z

Currently, I get this error in the browser console whenever I select a callee, which is self-explanatory and reflects the issue we are having:

[Vue warn]: Avoid mutating a prop directly since the value will be overwritten whenever the parent component re-renders. Instead, use a data or computed property based on the prop's value. Prop being mutated: "marketId"

found in

---> <CollateralAuctionSwapTransactionTable> at components/auction/collateral/CollateralAuctionSwapTransactionTable.vue
       <SwapTransaction> at components/auction/collateral/CollateralAuctionSwapTransaction.vue
         <SplitLayout> at components/layout/SplitLayout.vue
           <CollateralAuctionFlow> at components/auction/collateral/CollateralAuctionFlow.vue
             <AuctionsContainer> at containers/AuctionsContainer.vue
               <Pages/collateral.vue> at pages/collateral.vue
                 <Nuxt>
                   <Layouts/default.vue> at layouts/default.vue
                     <Root> [vue.runtime.esm.js:619](webpack:///node_modules/vue/dist/vue.runtime.esm.js?2b0e)

@valiafetisov, I can't seem to figure out how to avoid mutating the prop directly and syncing it at the same time.

Figured it out!

frontend/store/auctions.ts

frontend/components/auction/collateral/MarketPriceSelection.vue

LukSteib

(note that it should not block if we cannot figure out easily): On initial load of the transaction page none of the callees is indicated as "Selected". Only after first selecting one of them the indicator holds (see s1 below)
One more thing that makes me wonder: In the example below (ETH-A simulation). We preselect univ2 but a higher price is more favorable in this case. Similar: comparing gross and net profit we indicate that uniswapv3 would yield overall better outcome for the user. Just trying to understand how this is possible. (see comparison in s2 below)

s1

s2

aomafarag · 2022-11-07T11:00:40Z

(note that it should not block if we cannot figure out easily): On initial load of the transaction page none of the callees is indicated as "Selected". Only after first selecting one of them the indicator holds (see s1 below)

This was answered above

valiafetisov · 2022-11-07T11:58:55Z

This was answered above

Issue description doesn't imply that no price should be set as selected in the UI. I think we still need to indicate which one is currently "best" or "selected"

frontend/components/auction/collateral/MarketPriceSelection.vue

Co-authored-by: valia fetisov <[email protected]>

aomafarag added 7 commits October 12, 2022 17:06

Add types

9f9b329

Add new types to helpers/generateFakeAuction

edfd9be

Create MarketPriceSelection component and stories

8f3f23c

Emit from MarketPriceSelection to sync marketId

f211ed5

Initial implementation

4be5323

Adjust component layout and finalize implementation

3cbd019

Pass marketId down to the store

5112a99

LukSteib reviewed Oct 14, 2022

View reviewed changes

Change console.log to console.info to avoid lint error

4121563

LukSteib previously approved these changes Oct 17, 2022

View reviewed changes

Sort callees

cc19d85

aomafarag dismissed LukSteib’s stale review via cc19d85 October 19, 2022 08:32

valiafetisov reviewed Oct 19, 2022

View reviewed changes

Modify types

c6f85ca

LukSteib requested changes Oct 21, 2022

View reviewed changes

LukSteib previously approved these changes Oct 21, 2022

View reviewed changes

valiafetisov and others added 3 commits November 4, 2022 18:49

Merge branch 'main' into market-price-selection

596db9a

Reflect change in marketId in all related values

3fcc0e3

Remove duplicate type

d99db0e

aomafarag dismissed LukSteib’s stale review via d99db0e November 4, 2022 18:30

Modify stories to reflect callee selection

c9a3445

aomafarag added 2 commits November 5, 2022 20:19

Make market price selection persist

06265c1

Pass marketId up for execution

e2e5432

valiafetisov reviewed Nov 7, 2022

View reviewed changes

frontend/store/auctions.ts Outdated Show resolved Hide resolved

frontend/components/auction/collateral/MarketPriceSelection.vue Outdated Show resolved Hide resolved

LukSteib requested changes Nov 7, 2022

View reviewed changes

aomafarag self-assigned this Nov 7, 2022

Sort callees ascendingly

5a61905

valiafetisov reviewed Nov 7, 2022

View reviewed changes

frontend/components/auction/collateral/MarketPriceSelection.vue Outdated Show resolved Hide resolved

aomafarag and others added 2 commits November 7, 2022 13:12

Pre-select suggestedMarketId

48a31c0

Co-authored-by: valia fetisov <[email protected]>

Lint

c510d3f

valiafetisov approved these changes Nov 7, 2022

View reviewed changes

LukSteib approved these changes Nov 7, 2022

View reviewed changes

valiafetisov merged commit cbf5727 into main Nov 7, 2022

valiafetisov deleted the market-price-selection branch November 7, 2022 14:49

valiafetisov changed the title ~~Add MarketPriceSelection component and stories~~ Add multi-callee support to the frontend Nov 7, 2022

valiafetisov mentioned this pull request Nov 7, 2022

Extend multi-callee with auto router #527

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multi-callee support to the frontend #498

Add multi-callee support to the frontend #498

aomafarag commented Oct 13, 2022 •

edited

Loading

LukSteib left a comment

LukSteib left a comment

valiafetisov commented Oct 17, 2022 •

edited by aomafarag

Loading

aomafarag commented Oct 19, 2022

valiafetisov Oct 19, 2022

aomafarag Oct 19, 2022

aomafarag Oct 19, 2022

LukSteib left a comment

aomafarag commented Oct 21, 2022 •

edited

Loading

LukSteib commented Oct 21, 2022

aomafarag commented Nov 4, 2022 •

edited

Loading

LukSteib commented Nov 4, 2022

aomafarag commented Nov 5, 2022

LukSteib left a comment •

edited

Loading

aomafarag commented Nov 7, 2022

valiafetisov commented Nov 7, 2022

Add multi-callee support to the frontend #498

Add multi-callee support to the frontend #498

Conversation

aomafarag commented Oct 13, 2022 • edited Loading

LukSteib left a comment

Choose a reason for hiding this comment

LukSteib left a comment

Choose a reason for hiding this comment

valiafetisov commented Oct 17, 2022 • edited by aomafarag Loading

aomafarag commented Oct 19, 2022

valiafetisov Oct 19, 2022

Choose a reason for hiding this comment

aomafarag Oct 19, 2022

Choose a reason for hiding this comment

aomafarag Oct 19, 2022

Choose a reason for hiding this comment

LukSteib left a comment

Choose a reason for hiding this comment

aomafarag commented Oct 21, 2022 • edited Loading

LukSteib commented Oct 21, 2022

aomafarag commented Nov 4, 2022 • edited Loading

LukSteib commented Nov 4, 2022

aomafarag commented Nov 5, 2022

LukSteib left a comment • edited Loading

Choose a reason for hiding this comment

aomafarag commented Nov 7, 2022

valiafetisov commented Nov 7, 2022

aomafarag commented Oct 13, 2022 •

edited

Loading

valiafetisov commented Oct 17, 2022 •

edited by aomafarag

Loading

aomafarag commented Oct 21, 2022 •

edited

Loading

aomafarag commented Nov 4, 2022 •

edited

Loading

LukSteib left a comment •

edited

Loading