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

Codebase review #7

Merged
merged 10 commits into from
Mar 7, 2021
Merged

Codebase review #7

merged 10 commits into from
Mar 7, 2021

Conversation

w-i-n-s
Copy link
Contributor

@w-i-n-s w-i-n-s commented Feb 24, 2021

Review the whole codebase and fix errors

@w-i-n-s w-i-n-s marked this pull request as draft February 24, 2021 14:27
@w-i-n-s w-i-n-s marked this pull request as ready for review February 28, 2021 16:13
@w-i-n-s w-i-n-s changed the title WIP: Codebase review Codebase review Mar 2, 2021
Copy link
Owner

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

When I create a wallet on master and then run this version, there seem to be two problems:

  1. in the settings screen the other cosigner is forgotten
  2. the addresses on the addresses screen are incorrect

Maybe something went wrong in the NSUserDefaults refactor commit 57f5af5?

NthKey/View/Settings/AnnonceView.swift Outdated Show resolved Hide resolved
NthKey.xcodeproj/project.pbxproj Show resolved Hide resolved
@w-i-n-s
Copy link
Contributor Author

w-i-n-s commented Mar 3, 2021

When I create a wallet on master and then run this version, there seem to be two problems:

  1. in the settings screen the other cosigner is forgotten
  2. the addresses on the addresses screen are incorrect

Maybe something went wrong in the NSUserDefaults refactor commit 57f5af5?

@Sjors yes, wrong condition fixed in b5f119d

@Sjors
Copy link
Owner

Sjors commented Mar 3, 2021

Thanks, can you squash b5f119d into 57f5af5?

@Sjors
Copy link
Owner

Sjors commented Mar 4, 2021

Found another bug: the Show Mnemonic button no longer shows a popup.

@Sjors
Copy link
Owner

Sjors commented Mar 6, 2021

The show mnemonic button still doesn't work for me.

Also don't forget to squash the fix wrong condition check part of 4b3d24e into whichever commit created the bug. Unless it already existed on master.

@w-i-n-s
Copy link
Contributor Author

w-i-n-s commented Mar 7, 2021

@Sjors

Found another bug: the Show Mnemonic button no longer shows a popup.

Fixed by rollback d9c481f. Details in #14. On changing architecture to MVVM (according to #11) we also collect all possible errors/alerts and will use enumeration for displaying.

Additional details about how it works: 3rd example and details

@Sjors Sjors merged commit 0bacb66 into Sjors:master Mar 7, 2021
@github-pages github-pages bot temporarily deployed to github-pages March 7, 2021 16:04 Inactive
@w-i-n-s w-i-n-s deleted the codebase_review branch March 7, 2021 16:41
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.

None yet

2 participants