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: transactions live sync #466

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Conversation

lukeromanowicz
Copy link
Contributor

@lukeromanowicz lukeromanowicz commented Aug 22, 2023

Description

Resolves #446 and Resolves #96

Adds loading indicator for transactions that have not been synced yet by the middleware

Demo

Because the MDW was heavily optimized recently, it's almost impossible to get a not synced transaction and redirect to the details page in time. Therefor I simulated this behavior in the demo.

2023-08-22.11-00-26.mp4

Checklist:

  • I have read and followed the Contributing Guide
  • My change does not require a change to the documentation.

@github-actions
Copy link

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

Copy link
Collaborator

@janmichek janmichek left a comment

Choose a reason for hiding this comment

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

Looks solid! However I am not sure how to test it.
Added some minor code suggestions

)

onBeforeRouteLeave(
(_to, _from, next) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These params are not needed I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well... how else can I get to next without putting two placeholders first? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see now.

I quess this is not gonna work?

Suggested change
(_to, _from, next) => {
({next}) => {

But the _ prefix could be removed, right? Or why is there underscore?

Copy link
Contributor Author

@lukeromanowicz lukeromanowicz Aug 24, 2023

Choose a reason for hiding this comment

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

I'm afraid {next} won't work because the handler is passing each value as separate parameter. _ prefix is enforced by linter accordingly with best practices to mark unused yet necessary parameters with that prefix. Here is the docs: https://eslint.org/docs/latest/rules/no-unused-vars#argsignorepattern

@@ -52,11 +71,16 @@ export const useWebSocket = defineStore('webSocket', () => {
return
}
const parsedData = camelcaseKeysDeep(JSON.parse(data))
await processSocketMessage(parsedData)
if (parsedData.subscription === 'Object' && parsedData.payload?.hash === subscribedTransactionId.value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be explained via extra const binding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried my best but it was really hard to come up with the name for the first part half of that if statement

@@ -69,11 +67,16 @@ export const useTransactionDetailsStore = defineStore('transactionDetails', () =
transactionTypeData.value = null
}

function processTransactionUpdate(message) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the process prefix it's not really clear to me what it does or what it returns. Would it make sense to name it updateTransactionTypeData, or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named it this way to preserve consistency with the other process function but sure, I don't have any strong preferences here. Updated accordingly with your recommendation

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 to me. At the same time it looks like mdw improved sync speed so now it's basically not happening anymore but it would be still useful in case it takes longer for some cases.

@@ -52,11 +71,19 @@ export const useWebSocket = defineStore('webSocket', () => {
return
}
const parsedData = camelcaseKeysDeep(JSON.parse(data))
await processSocketMessage(parsedData)
const isSpecificEntityMessage = parsedData.subscription === 'Object'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain this condition? Is it correct or was it supposed to be a typeof check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct. You can subscribe to general entity type messages (e.g. Transactions, Microblocks...) or a specific entity e.g. transaction id. This is the second case. In this scenario the message.subscription is 'Object' and the payload is specific to the entity type you subscribed.

I struggled to figure out a good name for this so originally it was without extracting to a dedicated const.

Copy link
Collaborator

@janmichek janmichek left a comment

Choose a reason for hiding this comment

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

Thanks for explanation. Added one more hint, but approving in advance

@marc0olo
Copy link
Contributor

@michele-franchi @lukeromanowicz thanks! I am wondering if it might be possible to make the loading spinner even more dynamic. some of the data shown in the details can already be obtained before the mdw is synced if I remember correctly 🤔

but already way better than before, thanks! =)

@lukeromanowicz
Copy link
Contributor Author

@marc0olo we discussed that but it was hard to come up with a good design on short notice. This solution is a quick win and consistent with loading data everywhere else in the app. We'll for sure improve it once a better design is figured out and can be applied globally in the application.

@lukeromanowicz lukeromanowicz merged commit ec703ce into develop Aug 24, 2023
@lukeromanowicz lukeromanowicz deleted the feat/transactions-sync branch August 24, 2023 16:05
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.

Loading indicator of transactions waiting for mdw sync Reload transaction details data when it gets synced
4 participants