Unstoppable domain (disabled until tested); latest txns cache; bugfixes #22
Conversation
…d balances on home; cache latest events
…d balances on home; cache latest events
…t would push over the memo char limit
…ry resilient to extra spaces
| public var isValidMnemonic = false | ||
| public var wordsCount = 0 | ||
| public var isValidNumberOfWords = false | ||
| public var maxWordsCount = 0 | ||
| public var isValidForm: Bool { | ||
| isValidMnemonic && | ||
| (birthdayHeight.data.isEmpty || | ||
| (!birthdayHeight.data.isEmpty && birthdayHeightValue != nil)) | ||
| (birthdayHeight.isEmpty || |
There was a problem hiding this comment.
Consider adding a test for this that serves as documentation and red flag if a change break the expectations, since these kind of statements are really good vectors for form validation bugs in prod.
| // validate the seed | ||
| try mnemonic.isValid(state.importedSeedPhrase.data) | ||
| let cleanedPhrase = trimmedPhraseWords.joined(separator: " ") | ||
| try mnemonic.isValid(cleanedPhrase) |
There was a problem hiding this comment.
Why is the cleaning and validation performed again, is there a way of continuing when the form is invalid?
There was a problem hiding this comment.
This logic was adjusted to make entering seed phrase a bit more forgiving, particularly around whitespace errors. Previously, if a user entered any extra whitespace between words, this would be considered an error and result in validation failing. This seemed extreme for an issue that could reasonably be compensated for by removing extra whitespace. If this is a security concern, I can revert it. Just seemed like a simple UX enhancement to make.
| func validateMnemonic() -> NighthawkTextEditor.ValidationState { | ||
| self.isValidMnemonic ? .valid : .invalid(error: L10n.Nighthawk.ImportWallet.invalidMnemonic) | ||
| } | ||
|
|
||
| func validateBirthday() -> NighthawkTextFieldValidationState { | ||
| (self.birthdayHeight.data.isEmpty || | ||
| (!self.birthdayHeight.data.isEmpty && self.birthdayHeightValue != nil)) | ||
| (self.birthdayHeight.isEmpty || |
There was a problem hiding this comment.
Consider adding tests that track changes in the code that break the expectations of these validations. It will help avoiding form errors and time of manual testing
There was a problem hiding this comment.
Yes, automated testing is on my to-do list. I will start revamping the test suite in coming PRs
| public var isValidMnemonic = false | ||
| public var wordsCount = 0 | ||
| public var isValidNumberOfWords = false | ||
| public var maxWordsCount = 0 | ||
| public var isValidForm: Bool { | ||
| isValidMnemonic && | ||
| (birthdayHeight.data.isEmpty || | ||
| (!birthdayHeight.data.isEmpty && birthdayHeightValue != nil)) | ||
| (birthdayHeight.isEmpty || |
There was a problem hiding this comment.
This validation is duplicated on a function below. Also it doesn't seem to actually validate that the data contains a valid string that can be converted to BlockHeight
There was a problem hiding this comment.
If birthdayHeightValue in non-nil, it means we were able to construct a BlockHeight from the string value, so yes, in an indirect sort of way, it is validating that, but I will say that it is not particularly clearly expressed by this implementation, so I think per your other comment, I will make the BlockHeight property a computed property.
As to the duplication, this particular computed property indicates whether the entire form is valid. The other one is validation a single field and is used by the view to show an error state for that one field should it be invalid.
| state.birthdayHeight = redactedBirthday | ||
|
|
||
| if let birthdayHeight = BlockHeight(state.birthdayHeight.data), birthdayHeight >= saplingActivation { | ||
| if let birthdayHeight = BlockHeight(state.birthdayHeight), birthdayHeight >= saplingActivation { |
There was a problem hiding this comment.
This make me think that it's possible that you want birthdayHeightValue to be a computed property based on state.birthdayHeight.data instead
There was a problem hiding this comment.
Yeah, that might make more sense.
This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.
Author
Reviewer