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

Fix balance update delay - Closes #1539 #1609

Merged
merged 6 commits into from Jan 8, 2019

Conversation

michaeltomasik
Copy link
Contributor

What issue have I solved?

-- #1539

How have I implemented/fixed it?

How has this been tested?

Review checklist

@michaeltomasik michaeltomasik self-assigned this Dec 18, 2018
@michaeltomasik michaeltomasik added this to Open Pull Requests in Version 1.9.0 via automation Dec 18, 2018
Copy link
Contributor

@slaweet slaweet left a comment

Choose a reason for hiding this comment

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

The root cause is that
https://github.com/LiskHQ/lisk-hub/blob/9cf9bab061a430d2cf847828d02d6cfe6c00dea1/src/store/middlewares/account.js#L98

is not called with the same timeout delay as
https://github.com/LiskHQ/lisk-hub/blob/9cf9bab061a430d2cf847828d02d6cfe6c00dea1/src/store/middlewares/account.js#L118
is.

So one solution would be to wrap
https://github.com/LiskHQ/lisk-hub/blob/9cf9bab061a430d2cf847828d02d6cfe6c00dea1/src/store/middlewares/account.js#L98
in a timeout as well.

But IMO a better solution is to use the same solution as in followed accounts middleware. That is to check the store that contains last 10 blocks and update if there is a relevant transaction in last ten blocks.
https://github.com/LiskHQ/lisk-hub/blob/9cf9bab061a430d2cf847828d02d6cfe6c00dea1/src/store/middlewares/followedAccounts.js#L26-L31

It will result in some extra requests but we'll be surer that we have the latest data.

We can try to get rid of it when LiskHQ/lisk-sdk#2403 and core 1.4 that should be fixing this issues is in mainnet.

Copy link
Contributor

@slaweet slaweet left a comment

Choose a reason for hiding this comment

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

@@ -59,9 +59,9 @@ describe('Delegate Registration', () => {
cy.get('@tx').find(ss.transactionReference).should('have.text', '-');
cy.get('@tx').find(ss.transactionAmountPlaceholder).should('have.text', '-');
// TODO Unskip when #1539 is fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

The TODO comments should be removed.

@Efefefef
Copy link
Contributor

Efefefef commented Jan 4, 2019

@michaeltomasik Please change the arrow function to regular function declaration inside 'it'.
Cypress uses the context variable of 'this' to memorize variables

Copy link
Contributor

@Efefefef Efefefef left a comment

Choose a reason for hiding this comment

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

👍

@slaweet slaweet merged commit 312fb85 into 1.9.0 Jan 8, 2019
Version 1.9.0 automation moved this from Open Pull Requests to Merged Pull Requests Jan 8, 2019
@slaweet slaweet deleted the 1539-fix-balance-update-after-tx-confirmation branch January 8, 2019 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Version 1.9.0
  
Merged Pull Requests
Development

Successfully merging this pull request may close these issues.

None yet

3 participants