Skip to content

Conversation

@KatunaNorbert
Copy link
Member

@KatunaNorbert KatunaNorbert commented Feb 23, 2021

Fixes #330.

Changes proposed in this PR:

  • Allow changing price inside edit form

Screen Shot 2021-04-21 at 15 15 41

@vercel
Copy link

vercel bot commented Feb 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oceanprotocol/market/D5tH3yhSwJM3WaTvwQDWx35GaAGt
✅ Preview: https://market-git-feature-allow-changing-of-fixed-rate-b3bb56.vercel.app

@KatunaNorbert
Copy link
Member Author

KatunaNorbert commented Feb 24, 2021

I managed to get the exchangeId and make the setRate function working but the price does not update.
I'm not sure if I'm looking at the right price, but I think is the price from ddo:
Screenshot 2021-02-24 at 13 36 22

@kremalicious
Copy link
Contributor

This might be the first field where we need some activate/deactivate UI pattern. Because right now, we always save all fields when user hits submit. This was fine so far because we populate with existing values, and metadata updating is one transaction anyway.

Price editing introduces another transaction but we can't have 2 transactions when user did not intend to change the price. Same the other way around, user only wants to change price so there should be no metadata editing transaction.

So once we find a UI for selecting fields to edit, we can use it on all fields so users will be able to selectively change what they want, and based on selections we figure out which exact transactions to send.

@mihaisc
Copy link
Contributor

mihaisc commented Feb 24, 2021

If you do a quick price fetch before you will get the exchangeId in price.address (not ddo.price..address, the price from the provider) address: tokenExchange.exchangeID || '',
Another option would be to fetch the exchange directly await ocean.fixedRateExchange.searchforDT(dataTokenAddress, '1' ) and get the exchangeId from the first item in the returned array

@KatunaNorbert
Copy link
Member Author

I see, I used ocean.fixedRateExchange.searchforDT() and then on set price It says that the owner address I give is invalid
Screenshot 2021-02-24 at 17 14 50
Screenshot 2021-02-24 at 17 12 19

@KatunaNorbert
Copy link
Member Author

This might be the first field where we need some activate/deactivate UI pattern. Because right now, we always save all fields when user hits submit. This was fine so far because we populate with existing values, and metadata updating is one transaction anyway.

Price editing introduces another transaction but we can't have 2 transactions when user did not intend to change the price. Same the other way around, user only wants to change price so there should be no metadata editing transaction.

So once we find a UI for selecting fields to edit, we can use it on all fields so users will be able to selectively change what they want, and based on selections we figure out which exact transactions to send.

Can't we do those checks in the edit method so we don't have to change the UI?

@kremalicious
Copy link
Contributor

ocean.fixedRateExchange.searchforDT() is probably way slower than using price.address from useAsset() which the asset provider already has when user lands on asset details.

Then for the last argument in setRate() the current wallet account from the currently connected user has to be used. So accountId from useOcean().

@kremalicious
Copy link
Contributor

Can't we do those checks in the edit method so we don't have to change the UI?

That could also work, would imply checking every input value against existing DDO values, and then go from there to only update what has been changed by user

@KatunaNorbert
Copy link
Member Author

In my case accountId and exchangeOwner are the same so I get the same error

@kremalicious
Copy link
Contributor

then we might have found a bug somewhere across the stack but requires more testing, I mean it's probably the first time that ocean.js method is used. Feel free to push any code change you have already so everybody has same testing base

@mihaisc
Copy link
Contributor

mihaisc commented Feb 24, 2021

so the error in the image (invalid exchange owner) you get after calling setRate?

@KatunaNorbert
Copy link
Member Author

KatunaNorbert commented Feb 24, 2021

Yes, interestingly It worked with the wrong exchangeId, or at least it didn't throw an error

@mihaisc
Copy link
Contributor

mihaisc commented Feb 24, 2021

It seems that something doesn't work in ocean.js. Suggest you drop it, i'll take a look

Signed-off-by: mihaisc <mihai.scarlat@smartcontrol.ro>
Signed-off-by: mihaisc <mihai.scarlat@smartcontrol.ro>
@mihaisc
Copy link
Contributor

mihaisc commented Mar 3, 2021

One issue is with the new price component. If we have const { price } = useMetadata(ddo) it works on the asset teaser but it will not update in asset details when price is updated. If we have const { price } = useAsset() it will properly update in asset details but it will not work on the asset teaser.

Signed-off-by: mihaisc <mihai.scarlat@smartcontrol.ro>
@mihaisc
Copy link
Contributor

mihaisc commented Mar 3, 2021

The price is being updated , the issue is that aqua is not updating it. Issue created oceanprotocol/aquarius#403
image

@mihaisc mihaisc assigned mihaisc and unassigned mihaisc Mar 3, 2021
@mihaisc mihaisc added the Status: Blocked Blocked by dependency, platform requirement, etc (add comment to detail the reason) label Mar 3, 2021
@KatunaNorbert KatunaNorbert requested a review from mihaisc as a code owner March 30, 2021 13:12
@KatunaNorbert
Copy link
Member Author

KatunaNorbert commented Apr 15, 2021

Oh, my bad, was trying to update the price on dataset with dynamic price.
Retested on fixed price and works! Should make some changes to disabled the price update on datasets with pools

@mihaisc
Copy link
Contributor

mihaisc commented Apr 15, 2021

Yes, please add that

@KatunaNorbert
Copy link
Member Author

KatunaNorbert commented Apr 15, 2021

Disabled the price input for dataset with pool type price

@KatunaNorbert KatunaNorbert removed the Status: Blocked Blocked by dependency, platform requirement, etc (add comment to detail the reason) label Apr 15, 2021
@kremalicious
Copy link
Contributor

Quick UI testing:

  • the edit price field should not even show up on dynamic priced assets, should only show up on fixed price assets
    Screen Shot 2021-04-15 at 17 08 03

  • the edit price field should follow our pattern we use whenever we edit prices in the create pricing flow, otherwise hard to get what the number represents. So should mimic this, either in full showing the datatoken relationship, or just with the input + prefix:
    Screen Shot 2021-04-15 at 17 10 44

Other than that, actual functionality of changing the fixed price works like charm!

@kremalicious
Copy link
Contributor

kremalicious commented Apr 21, 2021

  • updating of FRE price works now for me, although I somehow get 3 transactions for the whole metadata editing now
  • confirmed we do not do other unnecessary transactions when price is not touched, or non-FRE asset
  • metadata editing screen for non-FRE assets is broken now sorry, my bad, I tested on a compute asset which I think only works with what we have in feature/compute

@KatunaNorbert
Copy link
Member Author

Hmm, I only have to confirm 2 transactions when changing the price and 1 when not.

@kremalicious
Copy link
Contributor

ok, 2 transactions it is indeed. Either I can't count or this switching between 10 different market versions in my browser leads to unexpected results. Should be all perfect now.

Immediately had this question regarding user transparency and since setRate is another transaction, it might be feasible to show some price history, just like the metadata history. But that's for another new feature.

Copy link
Contributor

@kremalicious kremalicious left a comment

Choose a reason for hiding this comment

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

awesome, all good. Pending decision to merge in Slack

@kremalicious kremalicious merged commit cf44ab7 into main Apr 22, 2021
@kremalicious kremalicious deleted the feature/allow-changing-of-fixed-rate-price branch April 22, 2021 10:55
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.

Allow changing of fixed-rate price

4 participants