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

Factor getAccountBalance into getCachedAccountBalance and fetchAccountBalances #2694

Merged
merged 3 commits into from
Jun 9, 2021

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Jun 8, 2021

Issue Number

Split off from #2684

Overview

  • Rename getAccountBalance to getCachedAccountBalance for clarity
  • Add fetchAccountBalances function for un-cached behaviour
  • fetchAccountBalances from listStakeKeys

Comments

Anviking added 2 commits June 8, 2021 14:55
1. Rename existing getAccountBalance to getCachedAccountBalance to make
its behaviour clearer.

2. Add new `fetchAccountBalances` to `NetworkLayer`.

3. Remove unused `ErrGetAccountBalance` error

The error wasn't used, so it was just a cause for confusion and
complexity.

4. Make listStakeKeys use use fetchAccountBalances instead of
`getCachedAccountBalance`.

getCachedAccountBalance will return 0 the first time it is called for a
new stake key. This might not be appropriate for listing stake keys.

Let's block and wait for new values from the node instead.

If we see any performance problems in the future we can re-consider / go
for a third option.
@Anviking Anviking force-pushed the anviking/ADP-721/uncached-rewards branch from 1988dcd to aa41945 Compare June 8, 2021 13:04
@Anviking Anviking self-assigned this Jun 8, 2021
-- account to the internal set of observed account, such that it will be
-- fetched later.

, fetchAccountBalances
Copy link
Contributor

Choose a reason for hiding this comment

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

occasionally I think that fetchRewardAccountBalances and getCachedRewardAccountBalance would be better here in spite of being long...

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 fair -> d81089b

Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

I like this decomposition. If we have a proof that it helps with flaky integration testing then it is great

@Anviking
Copy link
Member Author

Anviking commented Jun 8, 2021

Thanks for having a look.

If we have a proof that it helps with flaky integration testing then it is great

This wasn't intended to fix flaky tests, although that's a good point, this caching-behaviour could be involved 🤔

(This PR only changes the behaviour of listStakeKeys. Wallet reward balances still use the cached version.)

@Anviking
Copy link
Member Author

Anviking commented Jun 8, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Jun 8, 2021
2693: Allow specifying purpose for acc x pub r=paweljakubas a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->
adp-950

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] updated swagger
- [x] enable passing purpose
- [x] adjust core unit tests
- [x] add integration test
- [x] guard purpose with integration test  


# Comments

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


2694: Factor `getAccountBalance` into `getCachedAccountBalance` and `fetchAccountBalances` r=Anviking a=Anviking

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->

Split off from #2684 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Rename `getAccountBalance` to `getCachedAccountBalance` for clarity
- [x] Add `fetchAccountBalances` function for un-cached behaviour
- [x] `fetchAccountBalances` from `listStakeKeys`  


# Comments

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Pawel Jakubas <[email protected]>
Co-authored-by: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 8, 2021

Build failed (retrying...):

@Anviking
Copy link
Member Author

Anviking commented Jun 8, 2021

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 8, 2021

Canceled.

@Anviking Anviking force-pushed the anviking/ADP-721/uncached-rewards branch from d81089b to 3b8b160 Compare June 8, 2021 14:19
@Anviking
Copy link
Member Author

Anviking commented Jun 8, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Jun 8, 2021
2694: Factor `getAccountBalance` into `getCachedAccountBalance` and `fetchAccountBalances` r=Anviking a=Anviking

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->

Split off from #2684 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Rename `getAccountBalance` to `getCachedAccountBalance` for clarity
- [x] Add `fetchAccountBalances` function for un-cached behaviour
- [x] `fetchAccountBalances` from `listStakeKeys`  


# Comments

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 8, 2021

Build failed:

  src/Test/Integration/Framework/DSL.hs:2275:7:
  1) API Specifications, SHELLEY_STAKE_POOLS, STAKE_POOLS_JOIN_01rewards - Can join a pool, earn rewards and collect them
       expected a successful response but got an error: DecodeFailure "{\"code\":\"created_invalid_transaction\",\"message\":\"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (S (S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [DelegsFailure (WithdrawalsNotInRewardsDELEGS (fromList [(RewardAcnt {getRwdNetwork = Mainnet, getRwdCred = KeyHashObj (KeyHash \\\"2bde53d335518b59732afbeb458aab9e869c4781735c19852aacaa30\\\")},Coin 496746603)]))]}))))\"}"
       While verifying (Status {statusCode = 500, statusMessage = "Internal Server Error"},Left (DecodeFailure "{\"code\":\"created_invalid_transaction\",\"message\":\"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: HardForkApplyTxErrFromEra S (S (S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [DelegsFailure (WithdrawalsNotInRewardsDELEGS (fromList [(RewardAcnt {getRwdNetwork = Mainnet, getRwdCred = KeyHashObj (KeyHash \\\"2bde53d335518b59732afbeb458aab9e869c4781735c19852aacaa30\\\")},Coin 496746603)]))]}))))\"}"))

  To rerun use: --match "/API Specifications/SHELLEY_STAKE_POOLS/STAKE_POOLS_JOIN_01rewards - Can join a pool, earn rewards and collect them/"

Randomized with seed 567840892

#2415

@Anviking
Copy link
Member Author

Anviking commented Jun 8, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Jun 8, 2021
2694: Factor `getAccountBalance` into `getCachedAccountBalance` and `fetchAccountBalances` r=Anviking a=Anviking

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->

Split off from #2684 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Rename `getAccountBalance` to `getCachedAccountBalance` for clarity
- [x] Add `fetchAccountBalances` function for un-cached behaviour
- [x] `fetchAccountBalances` from `listStakeKeys`  


# Comments

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 8, 2021

Build failed:

Failures:

  src/Test/Integration/Scenario/API/Shelley/Wallets.hs:988:26:
  1) API Specifications, SHELLEY_WALLETS, WALLETS_UTXO_02 - Sending and receiving funds updates wallet's utxo.
       While verifying (Status {statusCode = 200, statusMessage = "OK"},Right (ApiWallet {id = ApiT {getApiT = WalletId {getWalletId = b0d938dada1aa6c2bcd4de4786fcae0d035f0963}}, addressPoolGap = ApiT {getApiT = AddressPoolGap {getAddressPoolGap = 20}}, balance = ApiWalletBalance {available = Quantity {getQuantity = 0}, total = Quantity {getQuantity = 0}, reward = Quantity {getQuantity = 0}}, assets = ApiWalletAssetsBalance {available = ApiT {getApiT = TokenMap (fromList [])}, total = ApiT {getApiT = TokenMap (fromList [])}}, delegation = ApiWalletDelegation {active = ApiWalletDelegationNext {status = NotDelegating, target = Nothing, changesAt = Nothing}, next = []}, name = ApiT {getApiT = WalletName {getWalletName = "Empty Wallet"}}, passphrase = Just (ApiWalletPassphraseInfo {lastUpdatedAt = 2021-06-08 19:29:07.940773 UTC}), state = ApiT {getApiT = Syncing (Quantity {getQuantity = Percentage {getPercentage = 257 % 625}})}, tip = ApiBlockReference {absoluteSlotNumber = ApiT {getApiT = SlotNo 691}, slotId = ApiSlotId {epochNumber = ApiT {getApiT = EpochNo {unEpochNo = 13}}, slotNumber = ApiT {getApiT = SlotInEpoch {unSlotInEpoch = 41}}}, time = 2021-06-08 19:27:20.2 UTC, block = ApiBlockInfo {height = Quantity {getQuantity = 252}}}}))
       Waited longer than 90s to resolve action: "Wallet balance is as expected".
       expected: Quantity {getQuantity = 1562000000}
        but got: Quantity {getQuantity = 0}

  To rerun use: --match "/API Specifications/SHELLEY_WALLETS/WALLETS_UTXO_02 - Sending and receiving funds updates wallet's utxo./"

  src/Test/Integration/Scenario/API/Shelley/Wallets.hs:1346:57:
  2) API Specifications, SHELLEY_WALLETS, NETWORK_SHELLEY - Wallet has the same tip as network/information
       Waited longer than 90s to resolve action: "Wallet has the same tip as network/information".
       expected: Ready
        but got: Syncing (Quantity {getQuantity = Percentage {getPercentage = 497 % 1250}})

  To rerun use: --match "/API Specifications/SHELLEY_WALLETS/NETWORK_SHELLEY - Wallet has the same tip as network/information/"

Cascade of more and more failures (137 in total), starting with the above.

Wallet syncing problems, starting before WALLETS_UTXO_02 failed. But if they caused the failure, or the other way around.

#2720

@Anviking
Copy link
Member Author

Anviking commented Jun 9, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 9, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 83f9af2 into master Jun 9, 2021
@iohk-bors iohk-bors bot deleted the anviking/ADP-721/uncached-rewards branch June 9, 2021 10: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.

2 participants