Skip to content
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

Add a getblocktemplate-rpcs feature to zebra-rpc #5305

Closed
Tracked by #5234
mpguerra opened this issue Sep 29, 2022 · 5 comments · Fixed by #5357
Closed
Tracked by #5234

Add a getblocktemplate-rpcs feature to zebra-rpc #5305

mpguerra opened this issue Sep 29, 2022 · 5 comments · Fixed by #5357
Assignees

Comments

@mpguerra
Copy link
Contributor

No description provided.

@oxarbitrage
Copy link
Contributor

It seems we have a few options here, not sure if they are written somewhere else.

It seems we don't need this feature in zebrad at least until we make a call to one of the new methods from an acceptance test.

We do need it in zebra-rpc so we can add new methods.

The Rpc trait implements all the methods so we can:

  • Add #[cfg(feature = "getblocktemplate-rpcs")] on top of each function we declare, on top of each function implementation and on top of each test that will use any of this new code. Or
  • We can add it once in the methods.rs file like
     #[cfg(feature = "getblocktemplate-rpcs")]
    pub mod getblocktemplate;
    

Adding it once and have all the code in a mod seems to be a bit better however i am not sure if we can easily add new methods to the base trait.

@teor2345
Copy link
Contributor

teor2345 commented Oct 5, 2022

It seems we don't need this feature in zebrad at least until we make a call to one of the new methods from an acceptance test.

We've added acceptance tests to each RPC ticket, so we'll probably want that feature at the same time.
(It's quick to add a new feature.)

We do need it in zebra-rpc so we can add new methods.

The Rpc trait implements all the methods so we can:

* Add `#[cfg(feature = "getblocktemplate-rpcs")]` on top of each function we declare, on top of each function implementation and on top of each test that will use any of this new code. Or

* We can add it once in the methods.rs file like
  ```
   #[cfg(feature = "getblocktemplate-rpcs")]
  pub mod getblocktemplate;
  ```

Adding it once and have all the code in a mod seems to be a bit better however i am not sure if we can easily add new methods to the base trait.

We could add a getblocktemplate module, with a:

  • GetBlockTemplateRpc supertrait, and
  • an impl GetBlockTemplateRpc for Rpc block?

https://doc.rust-lang.org/rust-by-example/trait/supertraits.html

Then if the feature is active, we automatically get the new trait impl, and if it's not, we don't.
(We'll have to check if this works with #[rpc(server)].)

@mpguerra
Copy link
Contributor Author

mpguerra commented Oct 6, 2022

@mpguerra
Copy link
Contributor Author

we need to implement at least one rpc call before we can "hide it" behind a feature

@oxarbitrage
Copy link
Contributor

we need to implement at least one rpc call before we can "hide it" behind a feature

The PR #5357 attempt to implement the getblockcount and the feature. getblockcount was selected as it is really easy an one. (just return the current chain tip height)

I think we should go that way (implementing the feature first) to avoid the refactor (moving code among files) that we will have to do if we use the supertrait design to implemnent this.

If we add the feature function by function then we add the feature code with each call but initially we want to try the other way as it is somehow cleaner.

@teor2345 teor2345 changed the title Add a getblocktemplate-rpcs feature to zebrad and zebra-rpc Add a getblocktemplate-rpcs feature to zebra-rpc Oct 19, 2022
@mergify mergify bot closed this as completed in #5357 Oct 19, 2022
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.

3 participants