fix(scrypt): Fix scrypt JSON parsing for p and r

[MatusKysel]

Description

Fixed the JSON parsing copy-paste bug so p and r are only read when their own keys are present, preventing defaults from being silently used. Updated logic in src/Keystore/ScryptParameters.cpp to check CodingKeys::SP::p and CodingKeys::SP::r instead of CodingKeys::SP::n.

[sergei-boiko-trustwallet]

Looks legit, thanks for the fix!

[Copilot]

Pull request overview

This PR fixes a critical copy-paste bug in the scrypt parameter JSON parsing logic. The bug caused the p and r parameters to always use default values when parsing JSON, even when different values were explicitly provided, because the code was incorrectly checking the n key instead of the respective p and r keys.

Changes:

• Fixed conditional checks for p parameter to use CodingKeys::SP::p instead of CodingKeys::SP::n
• Fixed conditional checks for r parameter to use CodingKeys::SP::r instead of CodingKeys::SP::n

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The fix correctly addresses the copy-paste bug where p and r parameters were checking the wrong JSON key (n instead of their respective keys). However, there is no test coverage that specifically validates the conditional parsing behavior when p or r keys are missing from JSON. Consider adding a unit test that verifies the default values are used when these keys are absent, and that custom values are correctly parsed when present.

[sergei-boiko-trustwallet]

@MatusKysel, from the first glance, the fix is correct, but the implementation in general looks wrong because we shouldn't use default n, p, r if they aren't provided. Should we throw an exception instead?
Am I missing something?

[MatusKysel]

@MatusKysel, from the first glance, the fix is correct, but the implementation in general looks wrong because we shouldn't use default n, p, r if they aren't provided. Should we throw an exception instead? Am I missing something?

Thanks for the catch. The fix here only corrects the key checks for p/r; the “default if missing” behavior was already present via the struct defaults (same pattern as PBKDF2 iterations). So the patch doesn’t introduce that behavior.

If we want to treat missing n/p/r as invalid, I agree that’s a separate behavior change. We’d probably want to enforce required fields (and likely do the same for PBKDF2) to avoid silently falling back and to surface malformed keystores explicitly. Happy to follow up with a separate PR if that’s the direction