Skip to content

Conversation

@jaoleal
Copy link
Collaborator

@jaoleal jaoleal commented May 23, 2025

What is the purpose of this pull request?

  • Bug fix
  • Documentation update
  • New feature
  • Test
  • Other: Quality of life change

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

We rely on BlockchainInterface and UpdatableChainState traits to define how we expect the backend chain to be but it can became kind of repetitive mentioning every single trait. This is a try to mitigate this.

Notes to the reviewers

Theres other ways to to different what i did here, that is, reducing trait bounds verbosity... Thoughts ?

Author Checklist

  • I've followed the contribution guidelines
  • I've verified one of the following:
    • Ran just pcc (recommended but slower)
    • Ran just lint-features '-- -D warnings' && cargo test --release
    • Confirmed CI passed on my fork
  • I've linked any related issue(s) in the sections above

Finally, you are encouraged to sign all your commits (it proves authorship and guards against tampering—see How (and why) to sign Git commits and GitHub's guide to signing commits).

@jaoleal jaoleal force-pushed the declare_chain_types branch 3 times, most recently from e79c18d to da7deaf Compare May 23, 2025 14:41
@JoseSK999
Copy link
Contributor

Seems a nice thing, in fact we use a similar trait grouping in florestad, the RpcChain trait. That one could be replaced with this one + Clone.

I would use a name like ChainBackend (for the simple two traits) and SharedChain (for the send+sync+'static).

Also I wouldn't replace the BlockchainInterface + UpdatableChainstate + 'static with the full send+sync one, instead I would use ChainBackend + 'static. Wdyt? @jaoleal @Davidson-Souza

@jaoleal jaoleal force-pushed the declare_chain_types branch from da7deaf to 6848f37 Compare May 23, 2025 16:12
@jaoleal
Copy link
Collaborator Author

jaoleal commented May 23, 2025

agreed, nice naming! applied on 6848f37

@jaoleal jaoleal marked this pull request as ready for review May 23, 2025 16:16
@jaoleal jaoleal force-pushed the declare_chain_types branch from 6848f37 to 80c6c72 Compare May 23, 2025 16:41
@Davidson-Souza Davidson-Souza added the code quality Generally improves code readability and maintainability label May 24, 2025
@jaoleal jaoleal force-pushed the declare_chain_types branch 3 times, most recently from 4fe2f93 to bce7340 Compare May 26, 2025 21:19
Copy link
Contributor

@JoseSK999 JoseSK999 left a comment

Choose a reason for hiding this comment

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

Only some final nits.

I would also replace the RpcChain trait defined in florestad/src/json_rpc/server.rs to this:

/// Utility trait to ensure that the chain in our RPC server implements all the necessary traits
pub trait RpcChain: ThreadSafeChain + Clone {}

impl<T: ThreadSafeChain + Clone> RpcChain for T {}

@jaoleal jaoleal force-pushed the declare_chain_types branch 2 times, most recently from 64e1558 to 1f1f612 Compare May 27, 2025 14:51
@jaoleal
Copy link
Collaborator Author

jaoleal commented May 27, 2025

Applied @JoseSK999 suggestions

@jaoleal jaoleal force-pushed the declare_chain_types branch from 1f1f612 to 7f5fffd Compare May 27, 2025 18:18
@jaoleal
Copy link
Collaborator Author

jaoleal commented May 27, 2025

Applied @JoseSK999 suggestions

Copy link
Contributor

@JoseSK999 JoseSK999 left a comment

Choose a reason for hiding this comment

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

ACK 7f5fffd

@Davidson-Souza
Copy link
Member

ACK 7f5fffd

@Davidson-Souza Davidson-Souza merged commit 34dcb07 into vinteumorg:master May 28, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Generally improves code readability and maintainability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants