Skip to content

types: 'status' column is not that great #109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tcharding opened this issue Mar 19, 2025 · 4 comments · Fixed by #142
Closed

types: 'status' column is not that great #109

tcharding opened this issue Mar 19, 2025 · 4 comments · Fixed by #142

Comments

@tcharding
Copy link
Member

There are a few problems with the 'status' column in the docs of each module e.g, types/src/v17/mod.rs. Currently:

  • done (untested)
  • done
  • omitted
  • todo

The 'omitted' status has an explicit meaning of, according to the docs in each module

//! **Items marked omitted were omitted because:**
//!
//! - Method does not return anything.
//! - Method returns a simple type (e.g. bool or integer).
//! - Method is deprecated.
  • (1) and (2) are not true anymore because we have started to have integration tests that check null and std types returned - this is useful because it catches changes to the RPC API (if a future version of Core starts returning a different json object).
  • Deprecated methods don't work unless a special flag is passed to Core on startup, currently we never do that.

Perhaps we should have this set of status keys:

  • returns null
  • std type
  • versioned type
  • version + model

And perhaps 'status' is the wrong word - 'support' might be better.

Is it worth the effort to wrap every std type e.g FooMethod(pub String) - I did this in some places and not others. The benefit is that we can then provide FooMethod(pub BlockHash) and implement into_model to save users having to do it. The downside is its a bunch more work - is it worth the effort?

@GideonBature
Copy link
Contributor

GideonBature commented Mar 28, 2025

  1. For the set of status keys, making it clearer is going to be great. To maintain consistency, I feel we should have:
  • returns null

  • returns std type

    both the null and std type should have the return keyword, to be more explicit on action + type.

  1. Secondly, I agree that support here sounds more appropriate compare to status. And I feel, if we are to consider using support, we can replace the following values:

    values replace by
    done supported
    done (untested) supported (untested)
    todo planned support
  2. I also feel it's worth the effort to wrap every std types, since you have done that already in some places, I can help to implement that in other places that are yet to be implemented. Since it will improve the users experience.

@tcharding
Copy link
Member Author

I have been thinking that the following might be good.

//! | JSON-PRC Method Name              | support |
//! |:----------------------------------|:-------:|
//! | getbestblockhash                  |  mtype  |  
//! | getblock                          |  vtype  |  
//! | getblockchaininfo                 |  std    |   
//! | getblockcount                     |  null   |  

(Just an example, these are not the types returned by these methods.)

Where mtype means 'has a model type and a version specific type' and vtype means 'has a version specific type only'. std and null as expected however I agree with you that wrapping would better. I have been lazy in that regard. Help much appreciated. I'm getting to a stage where I think I will have full support done soon so the 'status' as such shouldn't be needed. If integration tests do not exist it should then be considered a bug.

@GideonBature
Copy link
Contributor

I think this is much better. It keeps it short and very clear.

True, I will love to work on the wrapping std types. Can you please open a separate issue for it, or should I still link it with this issue?

@tcharding
Copy link
Member Author

Added #115

tcharding added a commit to tcharding/corepc that referenced this issue May 2, 2025
The module docs in each version specific module (e.g. `types::v17`) are
supposed to be the single source of truth for devs trying to work out
which methods are supported and how.

Make an effort to better present the information.

Note that a bunch docs for methods marked as TODO may be incorrect re
version vs version + model. This doesn't matter right now since they
will be checked when implemented and also TODO implies no guarantees (in
my mind anyway).

Note also issue rust-bitcoin#144. `verify` does not check the Returns column against
the `Method` constructor type.

Close: rust-bitcoin#109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants