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

(Bugfix) - #1312 Assets disappear when loading a safe#1333

Merged
mmv08 merged 9 commits intodevelopmentfrom
bug/1312-assets-fetch
Sep 15, 2020
Merged

(Bugfix) - #1312 Assets disappear when loading a safe#1333
mmv08 merged 9 commits intodevelopmentfrom
bug/1312-assets-fetch

Conversation

@mmv08
Copy link
Contributor

@mmv08 mmv08 commented Sep 9, 2020

This PR:

@github-actions
Copy link

github-actions bot commented Sep 9, 2020

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Sep 9, 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 3 0
Ignored 0 N/A
  • Result: ✅ success

  • Annotations: 3 total


[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods


Report generated by eslint-plus-action

@lukasschor lukasschor changed the title Bug 1312: Assets disappear when loading a safe (Bugfix) - 1312 Assets disappear when loading a safe Sep 9, 2020
@lukasschor lukasschor changed the title (Bugfix) - 1312 Assets disappear when loading a safe (Bugfix) - #1312 Assets disappear when loading a safe Sep 9, 2020
@ghost
Copy link

ghost commented Sep 9, 2020

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

@ghost
Copy link

ghost commented Sep 10, 2020

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

@ghost
Copy link

ghost commented Sep 14, 2020

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

Copy link
Contributor

@fernandomg fernandomg left a comment

Choose a reason for hiding this comment

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

🥇 this looks nice and clean, but I still see the list of assets cleaning up on the load.

@mmv08
Copy link
Contributor Author

mmv08 commented Sep 15, 2020

@fernandomg thanks, for some reason changes to the reducer replacing merge vs mergeDeep disappeared, I have just done them again, please re-test :)

@ghost
Copy link

ghost commented Sep 15, 2020

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

@mmv08
Copy link
Contributor Author

mmv08 commented Sep 15, 2020

@ghost
Copy link

ghost commented Sep 15, 2020

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

['safes', safeAddress],
makeSafe({ name: 'LOADED SAFE', address: safeAddress }),
(prevSafe) => prevSafe.merge(safe),
(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.

This is not fixing the problem. For me the tokens disappear on the first load, displaying Ether alone. Then after few seconds they appear again.

The mergeDeep is virtually fixing this if applied in line 80 [ADD_SAFE], because we are calling [ADD_SAFE] again after the app is initialized and this is deleting the token values,having to wait until they load again.
I say that this virtually fix it because it introduces another issue. If mergeDeep is applied in [ADD_SAFE] it will add the owners again and again on each reload making the list of owners grow on the same basis as the number of owners we have. They will be shown duplicate when signing a transaction or in settings section.

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 please take a look again
I changed the ADD_SAFE action to return the existing state and not modify it if the safe already exists. To solve it in a fundamental way, we would have to rethink how safe is loaded when you land on the safe page as there is clearly a concurrency problem between add/update safe actions

@ghost
Copy link

ghost commented Sep 15, 2020

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

@fernandomg fernandomg self-requested a review September 15, 2020 13:40
Copy link
Contributor

@fernandomg fernandomg left a comment

Choose a reason for hiding this comment

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

nice!

@ghost
Copy link

ghost commented Sep 15, 2020

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

@mmv08 mmv08 merged commit e000958 into development Sep 15, 2020
@mmv08 mmv08 deleted the bug/1312-assets-fetch branch September 15, 2020 14:30
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Force fetch assets when accessing "Assets" tab

3 participants