Skip to content
This repository has been archived by the owner on Jul 15, 2022. It is now read-only.

LL-8508 - Fix NFT merging in incremental sync + tests #1552

Merged
merged 2 commits into from
Dec 2, 2021
Merged

LL-8508 - Fix NFT merging in incremental sync + tests #1552

merged 2 commits into from
Dec 2, 2021

Conversation

ghost
Copy link

@ghost ghost commented Dec 2, 2021

Context (issues, jira)

LL-8508

Description / Usage

Fix an issue that would never remove NFT from your account once transferred.

Expectations

  • Test coverage: The changes of this PR are covered by test. Unit test were added with mocks when depending on a backend/device.
  • No impact: The changes of this PR have ZERO impact on the userland. Meaning, we can use these changes without modifying LLD/LLM at all. It will be a "noop" and the maintainers will be able to bump it without changing anything.

@ghost ghost self-requested a review as a code owner December 2, 2021 15:18
@vercel
Copy link

vercel bot commented Dec 2, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ledgerhq/ledger-live-common/3SvU2BdkZhm3uQQbBweyGihhnsBm
✅ Preview: https://ledger-live-common-git-ll-8508-fix-nft-removal-7e862f-ledgerhq.vercel.app

gre
gre reviewed Dec 2, 2021
View reviewed changes
src/families/ethereum/nft.test.ts Outdated
@@ -122,6 +197,17 @@ describe("gaspard NFT on ethereum", () => {
expect(resync.nfts).toEqual(account.nfts);
});

test("remove half NFTs will restore them with half operations", async () => {
const halfOps = Math.ceil(account.operations.length || 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const halfOps = Math.ceil(account.operations.length || 0);
const halfOps = Math.ceil(account.operations.length / 2);

@ghost ghost force-pushed the LL-8508/fix-nft-removal-from-state branch from e2789d6 to 2827832 Compare December 2, 2021 15:41
@ghost ghost force-pushed the LL-8508/fix-nft-removal-from-state branch from 2827832 to 1c7b6d4 Compare December 2, 2021 15:42
@gre gre merged commit 7c8f68e into master Dec 2, 2021
valpinkman added a commit that referenced this pull request Dec 3, 2021
@valpinkman
Merge branch 'master' of github.com:LedgerHQ/ledger-live-common
b73e7a5 
* 'master' of github.com:LedgerHQ/ledger-live-common:
  Fixes reconciliation + CI
  reconciliation to remove .nfts on disabling
  cli
  v21.19.1
  LL-8436 Update fillDeviceTransactionConfig for NFT (#1553)
  LL-8508 - Fix NFT merging in incremental sync + tests (#1552)
  DOGE v3 is go
@LFBarreto LFBarreto mentioned this pull request Dec 3, 2021
Merged
2 tasks
@ghost ghost deleted the LL-8508/fix-nft-removal-from-state branch December 6, 2021 15:00
@LFBarreto LFBarreto mentioned this pull request Dec 18, 2021
Merged
2 tasks
@LFBarreto LFBarreto mentioned this pull request Feb 14, 2022
Merged
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gre @lambertkevin