rpc: Implement signmessage

[emersonian]

Dependent on merging verifymessage (#357)

Resolves #68

[oxarbitrage]

Hi @emersonian, nice work!

I added a few optional comments that I think will help with readability and make it easier for other developers to understand the code. If you’d like me to apply some or all of these suggestions, just let me know. Thanks!

[oxarbitrage]

Optional: Consider placing these tests in a mod tests submodule for RPC methods. This could help keep things organized as the implementation grows.

[emersonian]

I'm probably misunderstanding this - aren't they already in mod tests? (see the line below)

Or are you suggesting a separate file or directory structure?

[emersonian]

Oh disregard I understand now, a submodule. I am happy to do this refactor across RPC method if we want to but I feel that should be a separate cleanup task to keep all RPC methods aligned in their code style.

[oxarbitrage]

I’d suggest using a separated file structure for this (similar to how Zebra does it), splitting implementation and tests across files/modules like:
https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/block.rs#L34

The main reason is maintainability: RPC method implementations (and their tests) can easily grow to hundreds or thousands of lines.

For example, this method file in Zallet is already fairly large and will likely keep growing:
https://github.com/zecrocks/zallet/blob/88325198308f5f49a3ed07538f4019b2f155010a/zallet/src/components/json_rpc/methods/get_raw_transaction.rs
(from #364)

[oxarbitrage]

Oh disregard I understand now, a submodule. I am happy to do this refactor across RPC method if we want to but I feel that should be a separate cleanup task to keep all RPC methods aligned in their code style.

It’s not a big deal for me either, but I actually think the opposite. Doing this now helps us avoid a larger refactor later, and it sets a pattern early for RPC methods going forward.

[emersonian]

Perhaps if the maintainers want tests in separate files, they can create an issue to document their desire for all of the tests to be refactored in this way? It feels like a dedicated cleanup PR and a code style decision.

I personally agree with you for cleanliness but don't want to veer too far off of this PR's intended feature path.

[oxarbitrage]

If you look at the existing RPC method files in zallet:
https://github.com/zcash/wallet/tree/main/zallet/src/components/json_rpc/methods

You’ll see that none of them currently include unit tests, except for the one you added:
https://github.com/zcash/wallet/blob/main/zallet/src/components/json_rpc/methods/verify_message.rs

So it is not a refactor, it is starting that way.

[emersonian]

Ah, got it. Okay I will update all of my PRs I like your philosophy of starting clean.

[oxarbitrage]

We maybe want to use the same test vectors zcashd uses?
https://github.com/zcash/zcash/blob/v6.11.0/src/wallet/test/rpc_wallet_tests.cpp#L224-L242

[emersonian]

Currently we don't have an integration testing framework for zallet, these zcashd tests require a wallet. I think we should create a new issue to develop an integration testing approach for zallet, then to come back and improve the test coverage in any stateful RPC methods that could benefit from a test wallet.

[oxarbitrage]

I know is a bit of a pain but i think we should use #364 (comment) to do that kind of testing (for now).

[emersonian]

Thanks for writing that up. I think we should prioritize an automated approach sooner than later, so that we don't burn a lot of collective hours as a team by running manual integration tests for many PRs, especially as features potentially have bounties associated with them.

I would love your feedback on this idea: #371

[oxarbitrage]

Optional: It might be possible to simplify this to avoid the found variable and the break.

[emersonian]

I can refactor to use iterators but it feels less clean to me.

I bounced this is off of Claude, here's its take on this: (which could be quite wrong)

---

The tricky part is that the loop body has fallible operations (map_err()?, ok_or_else()?) that need to propagate errors immediately. An iterator approach using find_map would look something like:

  let (account, metadata) = wallet
      .get_account_ids()
      .map_err(|e| LegacyCode::Database.with_message(e.to_string()))?
      .into_iter()
      .find_map(|account_id| {
          let meta = wallet
              .get_transparent_address_metadata(account_id, address)
              .map_err(|e| LegacyCode::Database.with_message(e.to_string()))
              .transpose()?;  // Option<Result> -> Result<Option> gymnastics
          let account = wallet
              .get_account(account_id)
              .map_err(|e| LegacyCode::Database.with_message(e.to_string()))
              .and_then(|a| a.ok_or_else(|| LegacyCode::Database.with_static("Account not
  found")));
          // Now we need to return Option<Result<_, _>> to signal
          // "found" vs "not found" vs "error", which gets messy
          ...
      })

The closure in find_map returns Option, but we need to distinguish three states: not found (None), found (Some(Ok(...))), and error (Some(Err(...))). That forces us into Option<Result<..>> territory, which I think ends up less readable than the current found + break pattern.

[emersonian]

Thank you for the review! I force-pushed changes for the comments that I marked as resolved, and left a few replies on the others.