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

[Fix] Import sync_node and chain_selector #375

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jaoleal
Copy link
Contributor

@jaoleal jaoleal commented Feb 15, 2025

What is the purpose of this pull request?

  • Bug fix
  • Documentation update
  • New feature
  • Test
  • Other:

Which crates are being modified?

  • floresta-chain
  • floresta-cli
  • floresta-common
  • floresta-compact-filters
  • floresta-electrum
  • floresta-watch-only
  • floresta-wire
  • floresta
  • florestad
  • Other: .

Description

This Changes only add sync_node and chain_selector to lib.rs of crates/floresta-wire/src/lib.rs so them can be detected by docs.

Notes to the reviewers

it was

image

and you can see the difference running cargo doc --open in 3aa7203

Checklist

  • I've signed all my commits
  • I ran just lint
  • I ran cargo test
  • I've checked the integration tests
  • I've followed the contribution guidelines
  • I'm linking the issue being fixed by this PR (if any)

@jaoleal jaoleal force-pushed the fix-floresta_wire_api_docs branch from 4c39b5b to adc16dd Compare February 15, 2025 19:04
@jaoleal
Copy link
Contributor Author

jaoleal commented Feb 15, 2025

Just added some other little changes that i was seeing on the go.

All docs related.

Comment on lines 18 to 23
#[cfg(not(target_arch = "wasm32"))]
pub use p2p_wire::address_man;
pub use p2p_wire::chain_selector;
#[cfg(not(target_arch = "wasm32"))]
pub use p2p_wire::mempool;
#[cfg(not(target_arch = "wasm32"))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these pub use p2p_wire::{modules} are not compiled for wasm32. I don't know the exact reason (cc @Davidson-Souza), but we would do the same if it's important.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't build for wasm because it doesn't have TcpSockets. We need to make this optional and allow to use something like websockets to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill add the feature gate.

how serious is this non-std thing ? I think its prudent to make some tracking issue or a discussion to expose the state of some crates being able to compile for non-std.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This crate doesn't work for no-std. I've added this so one can use floresta (the crate) on wasm but as long as they build their own transport.

@Davidson-Souza
Copy link
Collaborator

There's a genuine question to be made here: do we want to export ChainSelector and SyncNode? There's not much usage into using them in isolation, they are usually spawned by RunningNode.

@jaoleal
Copy link
Contributor Author

jaoleal commented Feb 17, 2025

There's a genuine question to be made here: do we want to export ChainSelector and SyncNode? There's not much usage into using them in isolation, they are usually spawned by RunningNode.

Even if one cant use them for something individual with the whole module itself, i do still think we should expose its docs as a floresta-wire module atleast...

@Davidson-Souza
Copy link
Collaborator

There's a genuine question to be made here: do we want to export ChainSelector and SyncNode? There's not much usage into using them in isolation, they are usually spawned by RunningNode.

Even if one cant use them for something individual with the whole module itself, i do still think we should expose its docs as a floresta-wire module atleast...

It would be easier to explain in the current docs all the stages, but without exporting them.

The current approach exposes them into the API. But leaking something that's not meant to be used is a bad practice.

Now that I'm thinking about it, the utreexo node should default to running node, so people don't need to care about it

@jaoleal
Copy link
Contributor Author

jaoleal commented Feb 25, 2025

Now that I'm thinking about it, the utreexo node should default to running node, so people don't need to care about it.

So, in the end we will actually remove the running node export ? (because its a internal of floresta-wire ? )
And in floresta-wire docs we explain its existence ?
And a follow up to

"the utreexo node should default to running node"

@Davidson-Souza
Copy link
Collaborator

Now that I'm thinking about it, the utreexo node should default to running node, so people don't need to care about it.

So, in the end we will actually remove the running node export ? (because its a internal of floresta-wire ? ) And in floresta-wire docs we explain its existence ? And a follow up to

"the utreexo node should default to running node"

Yes, this is what I'm pending towards. Is there a use-case where someone would say: "I just want to figure-out what's the best chain of headers", or "I have this chain of headers, please download and check every block on it and return"?

@jaoleal
Copy link
Contributor Author

jaoleal commented Feb 26, 2025

Is there a use-case where someone would say: "I just want to figure-out what's the best chain of headers", or "I have this chain of headers, please download and check every block on it and return"?

The only way to answer this question is reflecting what we want this project to became.

we already follow a modular approach to develop features here but, how modular we want them to be ? modular enough so one can take a module and its methods to use the way he wants ? IMO, yes.

@JoseSK999
Copy link
Contributor

JoseSK999 commented Feb 26, 2025

I think the chain selector seems more likely to be useful on its own (perhaps someone wants to use it but implement custom logic for the sync, e.g. a ZKP sync). But the ibd-and-then-return component doesn't seem useful on its own.

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 this pull request may close these issues.

3 participants