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(extension): [lw-10789] fix delegation tx summary #1245

Conversation

vetalcore
Copy link
Contributor

@vetalcore vetalcore commented Jun 25, 2024

Checklist

  • JIRA - LW-10789
  • Proper tests implemented
  • Screenshots added.

Proposed solution

Explain how does this PR solves the problem stated in JIRA ticket.
You can also enumerate different alternatives considered while approaching this task.

Testing

Describe here, how the new implementation can be tested.
Provide link or briefly describe User Acceptance Criteria/Tests that need to be met

Screenshots

Attach screenshots here if implementation involves some UI changes

@vetalcore vetalcore requested a review from mchappell June 25, 2024 11:09
@vetalcore vetalcore self-assigned this Jun 25, 2024
@vetalcore vetalcore requested a review from a team as a code owner June 25, 2024 11:09
@vetalcore vetalcore force-pushed the fix/lw-10789-fix-delegation-tx-summary branch from e6c56fa to 688870c Compare June 25, 2024 19:29
Copy link

github-actions bot commented Jun 25, 2024

Allure Report

allure-report-publisher generated test report!

smokeTests: ✅ test report for ac2dad7b

passed failed skipped flaky total result
Total 32 0 0 0 32

({ type, status }) =>
type &&
(type in DelegationActivityType ||
type === ConwayEraCertificatesTypes.Registration ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not add to the DelegationActivityType ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes an no, these are still conway era certs

@vetalcore
Copy link
Contributor Author

vetalcore commented Jun 26, 2024

i've run into the case when mapWalletActivities util returns cached results in case there is new pending tx due to resolve function checking only amount of inFlight transactions.

wallet-actiities-slice.ts

  (
    { addresses, transactions, assetInfo, delegation: { rewardsHistory } },
    { cardanoFiatPrice, fiatCurrency, assetId },
    { cardanoCoin, assetDetails }
  ) =>
    `${transactions.history.length}_${transactions.outgoing.inFlight.length}_${assetInfo.size}_${
      rewardsHistory.all.length
    }_${cardanoFiatPrice}_${fiatCurrency.code}_${assetId || ''}_${cardanoCoin?.id}_${assetDetails?.id}_${
      addresses[0]?.address
    }`

@mkazlauskas i'm wondering if there was a particular performance degradation you were trying to fix by introducing memoization here, or i could safely remove it for now?
@mchappell @ljagiela fyi.

@mkazlauskas
Copy link
Member

@mkazlauskas i'm wondering if there was a particular performance degradation you were trying to fix by introducing memoization here, or i could safely remove it for now?

Don't remember for sure, but I think yes there was. If I understand the issue correnctly, maybe you could do sth like transactions.outgoing.inFlight.map(({id}) => id).join()

@vetalcore vetalcore force-pushed the fix/lw-10789-fix-delegation-tx-summary branch from 688870c to ac2dad7 Compare June 27, 2024 09:33
Copy link

@vetalcore vetalcore merged commit 2a67a34 into fix/lw-10782-map-conway-era-certificates Jun 27, 2024
8 of 9 checks passed
@vetalcore vetalcore deleted the fix/lw-10789-fix-delegation-tx-summary branch June 27, 2024 09:36
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.

3 participants