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

feat: Oracles details events tab 2 #196

Merged
merged 31 commits into from
May 16, 2023
Merged

Conversation

janmichek
Copy link
Collaborator

@janmichek janmichek commented May 2, 2023

Description

resolves #16

followup #200

Demo

firefox_5BayglKkCB.mp4

Checklist:

@janmichek janmichek changed the title add orecleEvents UI feat: Oracles details events tab 2 May 2, 2023
@janmichek janmichek mentioned this pull request May 2, 2023
1 task
@github-actions
Copy link

github-actions bot commented May 2, 2023

Deployed to https://pr-196-aescan.stg.aepps.com

Copy link
Contributor

@lukeromanowicz lukeromanowicz left a comment

Choose a reason for hiding this comment

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

Looks good, and works perfectly. The code is also almost flawless 👍

src/components/OracleEventsTable.vue Show resolved Hide resolved
src/components/OracleEventsTable.vue Outdated Show resolved Hide resolved
@lukeromanowicz
Copy link
Contributor

One additional thing worth taking look at is the fee. In this example either the fee has been changed since the last query or we probably have wrong formatting on one of the values:
https://pr-196-aescan.stg.aepps.com/oracles/ok_qJZPXvWPC7G9kFVEqNjj9NAmwMsQcpRu6E3SSCvCQuwfqpMtN
image

Copy link
Contributor

@lukeromanowicz lukeromanowicz left a comment

Choose a reason for hiding this comment

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

Nice, job well done :)

@janmichek
Copy link
Collaborator Author

One additional thing worth taking look at is the fee. In this example either the fee has been changed since the last query or we probably have wrong formatting on one of the values: https://pr-196-aescan.stg.aepps.com/oracles/ok_qJZPXvWPC7G9kFVEqNjj9NAmwMsQcpRu6E3SSCvCQuwfqpMtN image

I discovered it's not a problem of formatting. Any ideas? SHould I consult with MDW team?

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

Well, at the end we decided to display only responses and I see the code is still requesting queries.

I would instead use
https://mainnet.aeternity.io/mdw/v2/oracles/ok_qJZPXvWPC7G9kFVEqNjj9NAmwMsQcpRu6E3SSCvCQuwfqpMtN/responses endpoint which provides query and response data
Unfortunately it does not provide any information about block hash or micro time so we will have to remove "Queried At" and "Respond At". All the other data is available in the endpoint.

@janmichek
Copy link
Collaborator Author

@michele-franchi Ok, will do. This was my original solution .)

@janmichek
Copy link
Collaborator Author

@michele-franchi
There is a expand parameter for the oracle detail, which I think I should use

https://mainnet.aeternity.io/mdw/v2/oracles/ok_R7cQfVN15F5ek1wBSYaMRjW2XbMRKx7VDQQmbtwxspjZQvmPM?expand

  1. Micro time and block height (I guess it's for response seems to be available)
  2. QueryTx = register_tx_hash and ResponseTx = extends[index].hash ... correct?

:is-collapsed="!isOpened.includes(index)"
@click="toggle(index)"/>
</td>
<td :class="[{'oracle-events-table__data--opened': isOpened.includes(index)}]">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for styling purposes of the last line, that should not be visible when details are open

@janmichek
Copy link
Collaborator Author

This was what confused me
5b86a63

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

Please have a look at my comments.

I also noticed the following:

image
  • It does not fit well on mobile when the response is expanded:
image

src/components/OracleEventsQueryPanel.vue Outdated Show resolved Hide resolved
<dd>{{ formatAePrice(event.queryFee) }}</dd>

<dt> Query:</dt>
<dd> {{ event.query }}</dd>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a base64 and should be decoded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

<dd>{{ event.responseTtl }}</dd>

<dt> Response:</dt>
<dd> {{ event.response }}</dd>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a base64 and should be decoded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting, the decode function is not working as expected. I will have to investigate.

src/components/OracleEventsQueryPanel.vue Outdated Show resolved Hide resolved
@janmichek
Copy link
Collaborator Author

The pagination is hidden because there are not enough elements. Actually there is no single oracle that would have that many events ./

@michele-franchi
Copy link
Collaborator

michele-franchi commented May 10, 2023

@janmichek
Copy link
Collaborator Author

@michele-franchi Right, sorry my fault, I overlooked this. Now I think I made it working.

@janmichek
Copy link
Collaborator Author

All fixed now, also with the UX

firefox_4Q14YMzipz.mp4

@janmichek janmichek requested a review from michele-franchi May 10, 2023 13:33
@janmichek
Copy link
Collaborator Author

@janmichek the code works well but knowing that this middleware endpoint is highly susceptible to failures I think we should somehow handle this situation in the frontend, especially since we don't have a proper error 500 page yet.

Thanks for another round, just give it a green light .)

Copy link
Contributor

@lukeromanowicz lukeromanowicz left a comment

Choose a reason for hiding this comment

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

Really nice, approving

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

It still does not look right to me, for example when I visit https://pr-196-aescan.stg.aepps.com/oracles/ok_2fVpBb8j58mNQcVjY7bwUVpBWZv6AySGDjqepNofU8XGGmEN53 I get different errors:

image

I think it's because the oracle does not have responses and this should be handled somehow. Also I would suggest to check with middleware why the endpoint is returning 500.

@janmichek
Copy link
Collaborator Author

It still does not look right to me, for example when I visit https://pr-196-aescan.stg.aepps.com/oracles/ok_2fVpBb8j58mNQcVjY7bwUVpBWZv6AySGDjqepNofU8XGGmEN53 I get different errors:
image

I think it's because the oracle does not have responses and this should be handled somehow. Also I would suggest to check with middleware why the endpoint is returning 500.

Found out this is a MDW issue aeternity/ae_mdw#1307 Not sure, how should we proceed @michele-franchi

@michele-franchi
Copy link
Collaborator

michele-franchi commented May 12, 2023

It still does not look right to me, for example when I visit https://pr-196-aescan.stg.aepps.com/oracles/ok_2fVpBb8j58mNQcVjY7bwUVpBWZv6AySGDjqepNofU8XGGmEN53 I get different errors:
image
I think it's because the oracle does not have responses and this should be handled somehow. Also I would suggest to check with middleware why the endpoint is returning 500.

Found out this is a MDW issue aeternity/ae_mdw#1307 Not sure, how should we proceed @michele-franchi

@janmichek
Even if it's a mdw issue this should be handled somehow, I don't expect to see the following if an API endpoint fails:
image

I'm not even talking from a UI perspective, I know we should handle errors properly but I don't expect the code to break.

@janmichek
Copy link
Collaborator Author

@michele-franchi Ok, got it. I created a predecessor of the proper solution #95 just for this entity, so we can reuse my solution later

firefox_H0DIgHlZ7x.mp4

@janmichek janmichek requested a review from michele-franchi May 15, 2023 08:54
Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

It still does not work, could you check again?
image

I see you implemented a way to handle errors from a UI perspective but in my last comment I simply suggested to handle errors from a code perspective.
Right now the code breaks and it executes slice on undefined. The simple solution would simply have been to not let the code break (avoid slice on undefined) as handling errors from a UI perspective is a wider topic but we can leave it like it is but please check the code 😄

@janmichek
Copy link
Collaborator Author

ou implemented a way to hand

That's another problem presented already in develop, not related to this implementation. I tried to discuss it here. How should we sanitize it? #222

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

It looks good now, let's also have a look at the bug 😄

Good job!

@janmichek
Copy link
Collaborator Author

It looks good now, let's also have a look at the bug 😄

Good job!

I will solve it separately right after. I will also verify with MDW

@janmichek janmichek merged commit c1cd7bb into develop May 16, 2023
@janmichek janmichek deleted the Oracles_details_events_tab_2 branch May 16, 2023 08:37
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.

Oracles details events tab
3 participants