zec
/
ZcashLightClientKit
mirror of https://github.com/zcash/ZcashLightClientKit.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Initial setup code review, lint and coding guidelines 

- bug report template
- feature request template
- ux report template
- Architecture readme
- swiftlint according to secant wallet
- local coverage information
- manual testing documentation
This commit is contained in:
Francisco Gindre 2021-09-14 11:39:14 -03:00
parent 80a30be6df
commit 23000d6306
12 changed files with 480 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
.github/ISSUE_TEMPLATE/bug-report.md vendored Normal file
View File

@ -0,0 +1,51 @@
---
name: Bug report
about: Create a report about a bug in Zcash iOS SDK.
title: ''
labels: 'bug'
assignees: ''
---
<!--
This issue tracker is only for technical issues related to Zcash iOS SDK.
General Zcash questions are best directed to the
Zcash Forum: https://forum.zcashcommunity.com/
For reporting security vulnerabilities or for sensitive discussions with our
security team, please email security@z.cash . You can use this GPG key to send
an encrypted message:
https://z.cash/gpg-pubkeys/security.asc
fingerprint: AF85 0445 546C 18B7 86F9 2C62 88FB 8B86 D8B5 A68C
The key and fingerprint are duplicated on our Public Keys page:
https://z.cash/support/pubkeys.html
-->
### Describe the issue
Please provide a general summary of the issue you're experiencing
### Can you reliably reproduce the issue?
#### If so, please list the steps to reproduce below:
1.
2.
3.
### Expected behaviour
Tell us what should happen
### Actual behaviour + errors
Tell us what happens instead including any noticable error output (any messages
displayed on-screen when e.g. a crash occurred)
<!-- Note: please do not include sensitive information. blur, scratch or annotate any
information like addresses, usernames, amounts or anything other that you might consider sensitive and it's not relevant to the problem you are reporting. -->
- App Version:
- iOS Version:
- Device: (if applies)
### Any extra information that might be useful in the debugging process.
<!-- Note: please do not include sensitive information. blur, scratch or annotate any
information like addresses, usernames, amounts or anything other that you might consider sensitive and it's not relevant to the problem you are reporting. -->

17
.github/ISSUE_TEMPLATE/bug_report.md vendored
View File

@ -1,17 +0,0 @@
---
name: Bug report
about: 'Tell us what''s wrong. '
title: ''
labels: ''
assignees: ''
---
**What is the bug?**
What happened, and why was it unexpected? If applicable, add screenshots to help explain your problem.
**Additional context**
Add any other context about the problem here. Device, OS, App versions, and other details appreciated.
**Solution**
Add fix suggestions here, or what you’d like to see.

22
.github/ISSUE_TEMPLATE/feature-request.md vendored Normal file
View File

@ -0,0 +1,22 @@
---
name: Feature request
about: Suggest an idea for the Zcash iOS SDK.
title: ''
labels: 'use case'
assignees: ''
---
## Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Example: I'm always
frustrated when [...]
## Describe the solution you'd like
A clear and concise description of what you want to happen.
## Alternatives you've considered
A clear and concise description of any alternative solutions or features you've
considered.
## Additional context
Add any other context or screenshots about the feature request here.

14
.github/ISSUE_TEMPLATE/feature_request.md vendored
View File

@ -1,14 +0,0 @@
---
name: Feature request
about: Suggest an idea.
title: ''
labels: ''
assignees: ''
---
**What is your feature request?**
A description of your feature request. References or screenshots appreciated.
**How would this feature help you?**
A clear and concise description of what this feature would help with, or do.

18
.github/ISSUE_TEMPLATE/ux-report.md vendored Normal file
View File

@ -0,0 +1,18 @@
---
name: UX report
about: Was this SDK hard to use? It's not you, it's us. We want to hear about it.
title: 'UX: '
labels: 'usability'
assignees: ''
---
<!-- Did the SDK not do what you expected?
Was it hard to figure out how to do something?
Could an error message be more helpful?
It's not you, it's us. We want to hear about it. -->
## What were you trying to do
## What happened

22
.github/pull_request_template.md vendored Normal file
View File

@ -0,0 +1,22 @@
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](../blob/main/CODE_REVIEW_GUIDELINES.md).
# Author
<!-- NOTE: Do not modify these when initially opening the pull request. This is a checklist template that you tick off AFTER the pull request is created. -->
- [ ] Self-review: Did you review your own code in GitHub's web interface? Code often looks different when reviewing the diff in a browser, making it easier to spot potential bugs.
- [ ] Automated tests: Did you add appropriate automated tests for any code changes?
- [ ] Code coverage: Did you check the code coverage report for the automated tests? While we are not looking for perfect coverage, the tool can point out potential cases that have been missed.
- [ ] Documentation: Did you update Docs as appropiate? (E.g [README.md](../blob/main/README.md), etc.)
- [ ] Run the app: Did you run the app and try the changes?
- [ ] Did you provide Screenshots of what the App looks like before and after your changes as part of the description of this PR? (only applicable to UI Changes)
- [ ] Rebase and squash: Did you pull in the latest changes from the main branch and squash your commits before assigning a reviewer? Having your code up to date and squashed will make it easier for others to review. Use best judgement when squashing commits, as some changes (such as refactoring) might be easier to review as a separate commit.
# Reviewer
- [ ] Checklist review: Did you go through the code with the [Code Review Guidelines](../blob/main/CODE_REVIEW_GUIDELINES.md) checklist?
- [ ] Ad hoc review: Did you perform an ad hoc review? _In addition to a first pass using the code review guidelines, do a second pass using your best judgement and experience which may identify additional questions or comments. Research shows that code review is most effective when done in multiple passes, where reviewers look for different things through each pass._
- [ ] Automated tests: Did you review the automated tests?
- [ ] Manual tests: Did you review the manual tests?_You will find manual testing guidelines under our [manual testing section](../blob/mater/docs/testing/manual_testing)_
- [ ] How is Code Coverage affected by this PR? _We encourage you to compare coverage befor and after your changes and when possible, leave it in a better place. [Learn More...](../blob/master/docs/testing/local_coverage.md)_
- [ ] Documentation: Did you review Docs, [README.md](../blob/master/README.md), [LICENSE.md](../blob/master/LICENSE.md), and [Architecture.md](../blob/master/docs/Architecture.md) as appropriate?
- [ ] Run the app: Did you run the app and try the changes? While the CI server runs the app to look for build failures or crashes, humans running the app are more likely to notice unexpected log messages, UI inconsistencies, or bad output data.

173
.swiftlint.yml
View File

@ -1,45 +1,144 @@
whitelist_rules:
- closure_spacing
- colon
- empty_enum_arguments
- fatal_error_message
- force_cast
- force_try
- force_unwrapping
- implicitly_unwrapped_optional
- legacy_cggeometry_functions
- legacy_constant
- legacy_constructor
- legacy_nsgeometry_functions
- operator_usage_whitespace
- redundant_string_enum_value
- redundant_void_return
- return_arrow_whitespace
- trailing_newline
- type_name
- unused_closure_parameter
- unused_optional_binding
- vertical_whitespace
- void_return
- custom_rules
# This SwiftLint file is based on this great guideline.
# https://github.com/raywenderlich/swift-style-guide
excluded:
- Carthage
- Pods
- ZcashLightClientKit/Service/ProtoBuf/compact_formats.pb.swift
- ZcashLightClientKit/Service/ProtoBuf/service.grpc.swift
- ZcashLightClientKit/Service/ProtoBuf/service.pb.swift
- Example
- ZcashLightClientKitTests
- xctemplates
colon:
apply_to_dictionaries: false
disabled_rules:
- notification_center_detachment
- orphaned_doc_comment
- todo
- unused_capture_list
- nesting # allow for types to be nested, common pattern in Swift
- multiple_closures_with_trailing_closure
- generic_type_name # allow for arbitrarily long generic type names
opt_in_rules:
- mark
- array_init
- attributes
- closure_end_indentation
- closure_spacing
- collection_alignment
- colon # promote to error
- convenience_type
- discouraged_object_literal
- empty_collection_literal
- empty_count
- empty_string
- enum_case_associated_values_count
- fatal_error_message
- first_where
- force_unwrapping
- implicitly_unwrapped_optional
- indentation_width
- last_where
- legacy_random
- literal_expression_end_indentation
- multiline_arguments
- multiline_arguments_brackets
- multiline_function_chains
- multiline_literal_brackets
- multiline_parameters
- multiline_parameters_brackets
- no_space_in_method_call
- operator_usage_whitespace
- overridden_super_call
- pattern_matching_keywords
- prefer_self_type_over_type_of_self
- redundant_nil_coalescing
- redundant_type_annotation
- return_arrow_whitespace
- strict_fileprivate
- toggle_bool
# - trailing_closure # weird in SwiftUI
- unneeded_parentheses_in_closure_argument
- unused_import
- vertical_whitespace_closing_braces
- vertical_whitespace_opening_braces
- weak_delegate
- yoda_condition
indentation: 2
custom_rules:
no_objcMembers:
name: "@objcMembers"
regex: "@objcMembers"
message: "Explicitly use @objc on each member you want to expose to Objective-C"
array_constructor:
name: "Array/Dictionary initializer"
regex: '[let,var] .+ = (\[.+\]\(\))'
capture_group: 1
message: "Use explicit type annotation when initializing empty arrays and dictionaries"
severity: warning
string_concatenation:
included: ".*\\.swift"
excluded: ".*Test\\.swift"
name: "String Concatenation"
regex: " \\+ \"|\" \\+ |\\+= \""
message: "Please use string interpolation instead of concatenation"
severity: error
print_function_usage:
included: ".*\\.swift"
excluded: ".*Test\\.swift"
name: "Swift print() or debugPrint() should not be used in App Code"
regex: "print\\(|debugPrint\\("
message: "The Swift print() or debugPrint() functions should not be used."
severity: warning
nslog_function_usage:
included: ".*\\.swift"
excluded: ".*Test\\.swift"
name: "Swift NSLog() should not be used in App Code"
regex: "NSLog\\("
message: "The swift NSLog function should not be used."
severity: error
attributes:
always_on_same_line:
- "@IBSegueAction"
- "@IBAction"
- "@NSManaged"
- "@objc"
force_cast: warning
force_try: warning
function_body_length:
warning: 150
legacy_hashing: error
identifier_name:
excluded:
- i
- id
- x
- y
- z
indentation_width:
indentation_width: 4
line_length:
warning: 150
ignores_urls: true
ignores_function_declarations: true
ignores_comments: true
file_length:
warning: 600
ignore_comment_only_lines: true
multiline_arguments:
first_argument_location: next_line
only_enforce_after_first_closure_on_first_line: true
private_over_fileprivate:
validate_extensions: true
trailing_whitespace:
ignores_empty_lines: true
ignores_comments: true
vertical_whitespace:
max_empty_lines: 2

48
CODE_REVIEW_GUIDELINES.md Normal file
View File

@ -0,0 +1,48 @@
# Overall:
- Does the contribution referece an existing Issue?
- Are the requirements well defined?
# Specification:
- Are the requirements for the change well specified? Where are they documented? Bugfixes need a clear specification of the bug's cause and fix. (Small PRs might specify in a github ticket. Large changes may require stand-alone docs, or ZIPs.)
- Is there a documented test plan, which describes how to manually verify the change works on testnet prior to a release?
# User Documentation:
- What is the "changelog" entry for this change? (All changes should include this.)
- Are there any changes which require updates to user-facing documentation? If so, does the new documentation make sense? (documentation should be placed in [docs/](/docs) folder of the repo)
# Testing:
- For non-minor PRs (up to the code reviewers and PR creator if this is needed), we require authors to perform a thorough self-review and self-test of the resulting code base, including use cases that might not be visible affected by the introduced changes. Reviewers are not expected to run the changes locally, but are definitely encouraged to do so at their best judgement.
- When introducing modifications that affect the UI, screenshots might be provided in a BEFORE/AFTER fashion to speed up UI/UX requirement validation.
- Are there new tests that check all of the requirements? If it's a bugfix does it include new tests testing the bug triggering condition? (Do they fail before the fix and pass after the fix?)
- Do tests include edge cases, error conditions, and "negative case" tests to ensure the software is robust? Example: a function for verifying transaction signatures should include a bunch of tests for invalid signature cases.
- Do tests include all "logical test coverage" in addition to requirements-focused tests? Logical test coverage verifies behavior of the code that isn't obvious or implied by the requirements themselves.
- Have all of the automated tests been executed by CI? Read over the CI report to verify this. (Note: our CI system may not currently provide test results prior to review. We should change this! In the meantime, the reviewer should run the tests.)
- Is it clear that the test plan does actually test the new change? Does the test plan include any edge case / negative case / error conditions?
# Code Review:
- Does the code structure make sense? Does it follow existing conventions / frameworks of existing code, or is it introducing new abstractions / structure?
- Is the code style consistent with the [Swift Style Guide](SWIFTLINT.md)?
- Does every change make sense? Make sure to ask questions, even or especially "basic" coding questions ("what does this mean in Swift?") and domain-newbie questions ("what is the priority system, and why does this line change the priority?").
In summary, here's a checklist that summarizes what reviewers should be looking for when doing a Code Review [1]
- The code is well-designed.
- The functionality is good for the users of the code.
- Any UI changes are sensible and look good.
- Any parallel programming is done safely.
- The code isn’t more complex than it needs to be.
- The developer isn’t implementing things they might need in the future but don’t know they need now or aren't using now.
- Code has appropriate unit tests.
- Tests are well-designed.
- The developer used clear names for everything.
- Comments are clear and useful, and mostly explain why instead of what.
- Code is appropriately documented (generally in g3doc).
- The code conforms to our style guides.
- Make sure to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
[[1] What to look for in a code review](https://google.github.io/eng-practices/review/reviewer/looking-for.html)

134
SWIFTLINT.md Normal file
View File

@ -0,0 +1,134 @@
# Swift Style Guide
The SwiftLint configuration on this repo is based on the great work of the raywenderlich.com folks [The Official raywenderlich.com Swift Style Guide](https://github.com/raywenderlich/swift-style-guide).
The purpose of this guide is to provide a base format for our Swift codebase. This guide describes what are the rules enforced by our swift lint rules and should beused when contributing or reviewing code.
These guides use SwiftLint as a standard. You can learn more about SwiftLint by visiting its [official documentation page](https://github.com/realm/SwiftLint).
## Table of Contents
* [Installing SwiftLint](#installing-swiftlint)
* [Using the configuration file](#using-the-configuration-file)
* [Xcode settings](#xcode-settings)
* [Running SwiftLint](#running-swiftlint)
* [Handling rule exceptions](#handling-rule-exceptions)
* [Approved exceptions](#approved-exceptions)
* [Implicitly unwrapped optionals](#implicitly-unwrapped-optionals)
* [Force cast](#force-cast)
* [Force unwrapping](#force-unwrapping)
* [SwiftUI and multiple trailing closures](#swiftui-and-multiple-trailing-closures)
* [Open-source files](#open-source-files)
* [Other notes](#other-notes)
## Installing SwiftLint
```bash
brew install swiftlint
```
If you are unable to use Homebrew, you may use one of the other methods described in the [SwiftLint documentation](https://github.com/realm/SwiftLint).
**Do not** install SwiftLint as a CocoaPod in your project.
## Xcode settings
You'll need to configure Xcode to remove trailing whitespace from all lines. This is not the default configuration.
In Xcode's Preferences, select **Text Editing** ▸ **Editing** and check **Including whitespace-only lines**.
![](images/trailing-whitespace.png)
## Handling rule exceptions
Your sample project must compile without warnings — SwiftLint or otherwise. In general, you should change your code to eliminate all warnings where necessary. When it comes to SwiftLint, however, there will be times when this isn't possible. In these situations, you'll need to use in-line comments to temporarily disable rules. You can find appropriate syntax to do this in [the SwiftLint documentation](https://realm.github.io/SwiftLint/#disable-rules-in-code).
You may only disable a rule if it is on the list of approved exceptions listed below.
Prefer the form that disables a rule only for the next line:
```
// swiftlint:disable:next implicitly_unwrapped_optional
var injectedValue: Data!
```
Or, similarly, for the previous line:
```
var injectedValue: Data!
// swiftlint:disable:previous implicitly_unwrapped_optional
```
If there are several approved exceptions, group them together and disable the rule for that region. Be sure to enable the rule after that section. Do not leave a rule disabled throughout the source file.
```
// swiftlint:disable implicitly_unwrapped_optional
var injectedValue1: Data!
var injectedValue2: Data!
// swiftlint:enable implicitly_unwrapped_optional
```
If you must disable rules in your project, leave those in-line comments in the project for the benefit of your teammates in the editing pipeline.
Finally, if you're not sure which rule is triggering a warning, you can find the rule name in parentheses at the end of message:
![](images/swiftlint-warning.png)
## Approved exceptions
There are certain common idioms that violate SwiftLint's strict checking. If you are unable to work around them — and you've already tried to find a better way to structure your code — you may disable rules as described in this section.
If you find that you're struggling with rules other than those described below, please reach out to your Team Lead with your specific example.
### Implicitly unwrapped optionals
It is sometimes common, in lieu of using dependency injection, to declare a child view controller's properties as implicitly unwrapped optionals (IUO). If you're unable to structure your project to avoid this, you may disable the `implicitly_unwrapped_optional` rule for those dependency declarations. With the advent of `@IBSegueAction`, this should be rare.
### Force cast
You may use force casting — and disable the `force_cast` rule — in the `UITableViewDataSource` and `UICollectionViewDataSource` methods that dequeue cells.
### Force unwrapping
You may use forced unwrapping — rule name `force_unwrap` — when returning a color from an asset catalog:
```
static var rwGreen: UIColor {
// swiftlint:disable:next force_unwrapping
UIColor(named: "rw-green")!
}
```
You may also use it in the same context as the force cast exception above, dequeuing cells in `UITableViewDataSource` and `UICollectionViewDataSource` methods.
Although it's preferred that you model appropriately defensive code for our readers, you may use force unwrapping to access resources that you _know_ are included in the app bundle.
Finally, you may use force unwrapping when constructing a `URL` from a hard-coded, and guaranteed valid, URL string.
### SwiftUI and multiple trailing closures
Idiomatic SwiftUI uses trailing closures to provide the view content for certain user interface elements. `Button` is a prime example; it has an initializer form that uses a closure to provide its `label`. It's common to write something like the following:
```
Button(action: { self.isPresented.toggle() }) {
Image(systemName: "plus")
}
```
This violates the rule that you should not use trailing closure syntax when a method accepts multiple closure parameters, so SwiftLint will flag it with a warning.
You can work around this by extracting the `Button`'s action into a separate method. While this is frequently a better solution when the action requires several statements, it's an onerous requirement when the action is a single statement, as in the example above.
In these cases, you're permitted to disable this rule **for the declaration of a SwiftUI view** only. The rule name is `multiple_closures_with_trailing_closure`.
### Open-source files
Occasionally, you'll find it necessary to include an unmodified open-source file in the sample project. It's a virtual certainty that these files won't comply with our style guide. Our practice has always been to leave these files in their original state. In this situation, you should disable SwiftLint for the entire file:
```
// swiftlint:disable all
```
## Other notes
While SwiftLint goes a long way towards making your source code compliant with our style guide, it doesn't cover everything. For example, it won't catch or force you to correct the formatting for multi-condition `guard` statements. If you find yourself butting heads with SwiftLint, or have improvement suggestions, please file an Issue and send a PR request with your suggestions.

6
docs/Architecture.md Normal file
View File

@ -0,0 +1,6 @@
# Architecture
To be Documented
Refer to [Current SDK Architecture on Android SDK](https://github.com/zcash/zcash-android-wallet-sdk#structure)

14
docs/testing/local_coverage.md Normal file
View File

@ -0,0 +1,14 @@
# Gathering Code Coverage
Although full coverage it's not our finality, we use this metric as a way to be aware of which code is being tested and to what extent.
## Checking the coverage locally
Once you run the tests locally you should be able to see the tests by going to the logs pane of the project navigation panel under the coverage section for that build run.
![](../../images/locating_coverage.png)
## Coverage results
Our advice is that you run coverage before and after implementing a feature, so that you can assess how the code and tests you developed affected coverage.
![](../../images/coverage_percent.png)

29
docs/testing/manual_testing/README.md Normal file
View File

@ -0,0 +1,29 @@
# Manual testing
We aim to automate as much as we possibly can. Still manual testing is really important for Quality Assurance.
Here you'll find our manual testing scripts. When developing a new feature you can add your own that provide the proper steps to properly test it.
## Running Darksidewalletd tests
1. clone [lightwalletd](https://github.com/zcash/lightwalletd.git)
`git clone https://github.com/zcash/lightwalletd.git`
2. on `lightwalletd`'s repo, compile and run lightwalletd in _darkside_ mode
```` bash
make
./lightwalletd --log-file /dev/stdout --darkside-very-insecure --darkside-timeout 1000 --gen-cert-very-insecure --data-dir . --no-tls-very-insecure
````
3. open Demo App workspace `ZcashLightClientSample.xcworkspace`
4. go to the manage schemes section
5. verify that the `ZcashLightClientKit-Unit-Tests` scheme is shown and shared
6. run the `AdvancedReOrgTests` test suite
## Running DerivationTool tests
1. open Demo App workspace `ZcashLightClientSample.xcworkspace`
2. go to the manage schemes section
3. verify that the `ZcashLightClientKit-Unit-DerivationToolTests` scheme is shown and shared
6. run the `ZcashLightClientKit-Unit-DerivationToolTests` test suite