Patch hashes and update the code #1477
Conversation
6cf4582 to
e3f6e58
Compare
|
Hmm, do we really need |
|
I don't know what was the initial motivation for adding the dependency test but git blame tells me it was added by @elichai in |
|
PR looks good to me. |
|
Yes, IIRC we had a bug in the one of the macros, that it assumed some things were available(don't remember if it was a dependency it assumed or something else) but they weren't when the macro was used from outside of the crate. |
|
Oh, wow, I thought this is not needed because integration tests were supposed to act exactly as if they used the library as a dependency but if they have some dependencies available that are used by macros I can totally see how the issue wouldn't be caught. That being said, I don't think the test can check anything unless it actually calls the macro. Do you know which one it was? |
|
We should be able to have an integration test that only uses |
|
@tcharding yeah, I think that's what we should do. My suggestion is to remove/disable the current test for now to get this in and then we can do the change in parallel with other. |
882f730 to
0cdc61f
Compare
This patches `bitcoin_hashes` to use the version in the repository and fixes the code after removal of `Deref`. This also turns off `AS_DEPENDENCY` check with the intention to refactor it later.
0cdc61f to
6acf9ac
Compare
|
The latest push has those tests disabled. |
| ), | ||
| prevouts: common_cache.prevouts.hash_again(), | ||
| sequences: common_cache.sequences.hash_again(), | ||
| outputs: common_cache.outputs.hash_again(), |
There was a problem hiding this comment.
Hah, good memory! I totally forgot we added this hash_again method.
There was a problem hiding this comment.
Heh, I stared at it confused for a few minutes thinking "is this doing the same thing and we forgot to change it or does it do something else?!"
There was a problem hiding this comment.
I think we added it specifically for this code, and then forgot to use it.
There was a problem hiding this comment.
Yeah, I think the big advantage of having the crates in the same repo is we can use new APIs like this immediately which should prevent us from forgetting.
This patches
bitcoin_hashesto use the version in the repository and fixes the code after removal ofDeref.