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

[deps] Update electrsd to 0.32.0 #120

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

Conversation

ValuedMammal
Copy link
Contributor

@ValuedMammal ValuedMammal commented Mar 18, 2025

Updates electrsd dev dependency to 0.32.0

Updated pinned deps needed to build on rust 1.63.0

fixes #119
fixes #117

@ValuedMammal ValuedMammal self-assigned this Mar 18, 2025
@coveralls
Copy link

coveralls commented Mar 18, 2025

Pull Request Test Coverage Report for Build 14040929389

Details

  • 135 of 135 (100.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.7%) to 87.48%

Files with Coverage Reduction New Missed Lines %
src/lib.rs 3 95.93%
Totals Coverage Status
Change from base Build 13912069194: -0.7%
Covered Lines: 1090
Relevant Lines: 1246

💛 - Coveralls

@ValuedMammal ValuedMammal marked this pull request as ready for review March 18, 2025 17:33
@ValuedMammal ValuedMammal changed the title wip,deps: bump electrsd to 0.32 [deps] Update electrsd to 0.32 Mar 18, 2025
@ValuedMammal
Copy link
Contributor Author

For unit tests

  • using deserialize_hex instead of FromHex
  • add back AddressType::Legacy

@ValuedMammal ValuedMammal changed the title [deps] Update electrsd to 0.32 [deps] Update electrsd to 0.32.0 Mar 18, 2025
Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK 0e02d79

assert_eq!(tx_info.status.block_height, tx_res.info.blockheight);
assert_eq!(tx_info.status.block_hash, tx_res.info.blockhash);
assert_eq!(tx_info.status.block_time, tx_res.info.blocktime);
// TODO(corepc): No .block_height field on GetTransaction ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, it's unfortunate, might take the opportunity and add it too as I need to finish my work on adding the other mining RPCs required for the BDK upgrade.

Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

tACK 0e02d79

It looks pretty straight-forward, with the two caveats of block_height and block_time fields.

src/lib.rs Outdated
tx_info.status.block_hash.map(|hash| hash.to_string()),
tx_res.block_hash
);
assert!(tx_info.status.block_time >= Some(tx_res.time as u64));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to assert with >= and against tx.time instead ?

I've tried to use the available block_time, but didn't understand why it now keeps being off-by-one 🤔.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was confused by that too, might need investigating.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have some suggested changes here: oleonardolima@e732a5b, that reduce a little the usage of parsing, relying on corepc methods instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW using the block_time now seems to be working just fine, I guess the off-by-one error I was getting was related to the block_height, which fetching by block_hash solves the issue.

- use specific methods such as: `into_model()`, and `block_hash()` to
  improve the usage of corepc client and it's types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants