Skip to content

Updating dependencies #377

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

Merged
merged 3 commits into from
Aug 24, 2019

Conversation

elichai
Copy link
Contributor

@elichai elichai commented Aug 23, 2019

Updating rust-bitcoin to latest 0.20, and rust-secp256k1 to 0.15.
Removed bitcoin_hashes and reused the one that is now re-exported from rust-bitcoin (rust-bitcoin/rust-bitcoin#289)

I rebased my wasm tests on top of this and updated the test and I can verify that after this the code here 100% supports wasm :)
https://github.com/elichai/rust-lightning/tree/wasm/wasm-test

One thing that a bit bothered me and should be double checked is that we only use usize for the weight, not for the fees themselves. (otherwise it won't be enough for 32 bit systems)

cc rust-bitcoin/rust-bitcoin#298

@elichai
Copy link
Contributor Author

elichai commented Aug 23, 2019

Maybe replacing as u64 with u64::from() can be a good idea and will promise us that we won't demote any integer and lose bits

@elichai elichai force-pushed the 2019-08-update-deps branch from 142ae31 to 6b43d77 Compare August 24, 2019 00:11
@codecov
Copy link

codecov bot commented Aug 24, 2019

Codecov Report

Merging #377 into master will increase coverage by <.01%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
+ Coverage   87.48%   87.48%   +<.01%     
==========================================
  Files          29       29              
  Lines       15775    15791      +16     
==========================================
+ Hits        13801    13815      +14     
- Misses       1974     1976       +2
Impacted Files Coverage Δ
src/chain/transaction.rs 100% <ø> (ø) ⬆️
src/util/macro_logger.rs 76% <ø> (ø) ⬆️
src/ln/onion_utils.rs 95.96% <ø> (+0.01%) ⬆️
src/chain/keysinterface.rs 95.74% <ø> (ø) ⬆️
src/ln/channelmanager.rs 81.57% <ø> (+0.01%) ⬆️
src/ln/chan_utils.rs 99.21% <ø> (ø) ⬆️
src/ln/peer_handler.rs 31.81% <ø> (ø) ⬆️
src/util/chacha20poly1305rfc.rs 97.95% <ø> (ø) ⬆️
src/util/test_utils.rs 53.33% <ø> (ø) ⬆️
src/lib.rs 100% <ø> (ø) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab8f5a8...2030206. Read the comment docs.

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Aug 24, 2019 via email

@elichai
Copy link
Contributor Author

elichai commented Aug 24, 2019

hmm the point of using bitcoin::hashes is for future robustness, but if you prefer I can revert this part.
anyhow, rust-bitcoin propagates this feature to bitcoin_hashes https://github.com/rust-bitcoin/rust-bitcoin/blob/master/Cargo.toml#L18

As a middle ground, we can in the code use bitcoin::hashes but also in the Cargo.toml import it to make sure we propagate the feature (although then in the future it can get out of sync without anyone noticing)

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Aug 24, 2019 via email

@elichai elichai force-pushed the 2019-08-update-deps branch from 6b43d77 to 2030206 Compare August 24, 2019 15:06
@elichai
Copy link
Contributor Author

elichai commented Aug 24, 2019

Ok, I changed it back to use bitcoin_hashes

@TheBlueMatt TheBlueMatt merged commit 39b562e into lightningdevkit:master Aug 24, 2019
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.

2 participants