Conversation
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub(crate) async fn encrypt_and_store_legacy_seed( |
There was a problem hiding this comment.
I NACKed this change in #109 because at the time I thought that what is now #152 would be merged shortly, meaning that this change (and the other it proposed) was creating unnecessary work. Ironically we are now in a position where #152 is likely to be merged shortly, but I'm now less averse to this change as long as we do indeed remember to do the additional work to revert it once #152 merges.
| os: ubuntu-latest | ||
| rustflags: '--cfg zcash_unstable="nu7"' | ||
| # TODO: Enable NU7 tests when zebra-chain used revision supports it. | ||
| #rustflags: '--cfg zcash_unstable="nu7"' |
There was a problem hiding this comment.
This change I disagree with. It is entirely expected that these test cases be allowed to fail and the PR can still be merged; that is why the required-checks job doesn't check this one. It would be no different if macOS or Windows builds started failing; we would permit PRs that break Zallet on those platforms to be merged because they are not (currently) target platforms.
The current test failure is actually a Zebra bug: Zebra should be using #[cfg(zcash_unstable="nu7")] annotations on NU7-specific code instead of commenting them out.
There was a problem hiding this comment.
Makes sense, thanks. Deferred the feature issue to Zebra. ZcashFoundation/zebra#9811
2 participants
I think it would be beneficial for external reviewers and contributors if the CI passed, even at the cost of temporarily reducing coverage. Coverage can be restored once the necessary conditions are met again. Right now, it’s difficult to tell whether a PR is introducing new lints, since the failing checks are effectively ignored.
1602d6c - Allowed dead code for a function. This function is actually used in Add
migrate_zcashd_walletcommand. #152, but that PR has been in draft for a long time and will need updates anyway. I’d prefer to remove the dead code annotation there instead.3a6d0cb - For the NU7 tests, Zebra currently doesn’t support them (see https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/parameters/network_upgrade.rs#L523). I think it would be better to restore these tests once we pin to the right revision, and remove them for now.Deferred to Zebra: Add azcash_unstableflag for NU7 ZcashFoundation/zebra#9811