Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Fix) (development) Missing collectibles and Safe version#1375

Merged
dasanra merged 11 commits intodevelopmentfrom
fix/missing-NFTs
Sep 23, 2020
Merged

(Fix) (development) Missing collectibles and Safe version#1375
dasanra merged 11 commits intodevelopmentfrom
fix/missing-NFTs

Conversation

@fernandomg
Copy link
Contributor

@fernandomg fernandomg commented Sep 18, 2020

This PR fixes an error in the current development branch where NFTs got missing and the safe's version was not set.

missing collectibles missing safe version

@fernandomg fernandomg added the Bug 🐛 Something isn't working label Sep 18, 2020
@fernandomg fernandomg self-assigned this Sep 18, 2020
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Sep 18, 2020

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Sep 18, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

makeSafe({ name: 'LOADED SAFE', address: safeAddress }),
(prevSafe) => prevSafe.merge(safe),
// `mergeDeep` because with `merge` nested data gets overwritten
(prevSafe) => prevSafe.mergeDeep(safe),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding this mergeDeep activates the owner list multiply bug. You can check it going to settings/owners and try to full reload the app.

Also visible in transaction list when expanding details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch! 🤦‍♂️

Copy link
Contributor Author

@fernandomg fernandomg Sep 21, 2020

Choose a reason for hiding this comment

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

Dani, I may have a working solution but ended up solving it using a sleep, just for testing purposes. I guess that with a piece of context information (a flag) I will have it properly working.

Will upload the changes tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dasanra, well I ended up with the not so elegant solution.

I'll later create a draft PR with the current status my work on fixing the loading process. I need to make a final review of my notes.

BTW, I think it's about time to start thinking about adding XState, as Nico suggested. This is a really good scenario to start with it. Thoughts?

@ghost
Copy link

ghost commented Sep 22, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

@fernandomg fernandomg requested a review from dasanra September 22, 2020 01:08
@ghost
Copy link

ghost commented Sep 22, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

fernandomg and others added 3 commits September 22, 2020 17:13
* fix address being used as name

* Restore ENS name when sending transaction

* use `addressName` as default value if it happens that the name in the addressBook is not defined

* use resolvedAddress to filter by address in the address book

Co-authored-by: Daniel Sanchez <daniel.sanchez@gnosis.pm>
Co-authored-by: Mati Dastugue <matias.dastugue@altoros.com>
* use `safeFeaturesEnabled` selector

also organized a bit the code (styles) and added Types for the `ChooseTxType` component

* fix `getGnosisSafeInstanceAt` return type

* add types to `safeStorage`

refactor `getSafeName`

* use redux selector to obtain master contract version

* fix return type

Co-authored-by: Daniel Sanchez <daniel.sanchez@gnosis.pm>
@ghost
Copy link

ghost commented Sep 22, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

1 similar comment
@ghost
Copy link

ghost commented Sep 22, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

@fernandomg
Copy link
Contributor Author

fernandomg commented Sep 22, 2020

Found that by loading a Safe with the URL, Owners take a while to appear in the Settings tab (~15s).

This is because owners won't be loaded to the redux if there's no Safe information stored in the localStorage.

The fix is in the commit 9f10f43 (#1375)

@ghost
Copy link

ghost commented Sep 22, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Sep 23, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

@ghost
Copy link

ghost commented Sep 23, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

1 similar comment
@ghost
Copy link

ghost commented Sep 23, 2020

Travis automatic deployment:
https://pr1375--safereact.review.gnosisdev.com/app

@dasanra dasanra merged commit a764a23 into development Sep 23, 2020
@dasanra dasanra deleted the fix/missing-NFTs branch September 23, 2020 12:39
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Bug 🐛 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants