Test protocol using elements-consensus rather than blockchain nodes#34
Conversation
luckysori
force-pushed
the
elements-consensus
branch
from
ba05fa3 to
94dc5f3
Compare
| use tokio::sync::Mutex; | ||
|
|
||
| #[tokio::test] | ||
| async fn borrow_and_repay() { |
There was a problem hiding this comment.
@thomaseizinger I've rewritten this test to not use testcontainers, which makes it so much faster. What do you think? Should we do it for the rest and transition to this kind of testing?
There was a problem hiding this comment.
Your decision :)
To build more confidence with the lib, it may be good to include some negative tests as well. That way we know it is doing something!
There was a problem hiding this comment.
Having said that, fast and high-confidence unit tests for protocols are a little dream coming true. Can't believe it was so easy to do. So much time wasted in the past 😭
| let hash = elements::hashes::hash160::Hash::hash(&pk.serialize()); | ||
| let script = Builder::new() | ||
| .push_opcode(opcodes::all::OP_DUP) | ||
| .push_opcode(opcodes::all::OP_HASH160) | ||
| .push_slice(&hash.into_inner()) | ||
| .push_opcode(opcodes::all::OP_EQUALVERIFY) | ||
| .push_opcode(opcodes::all::OP_CHECKSIG) | ||
| .into_script(); |
There was a problem hiding this comment.
Making an address from a key should give you this script I think.
There was a problem hiding this comment.
Thanks for the tip! Funnily enough for a p2wpkh address you have to construct the corresponding p2pkh address and call script_pubkey() on it. Otherwise it doesn't work!
| use tokio::sync::Mutex; | ||
|
|
||
| #[tokio::test] | ||
| async fn borrow_and_repay() { |
There was a problem hiding this comment.
Your decision :)
To build more confidence with the lib, it may be good to include some negative tests as well. That way we know it is doing something!
| use tokio::sync::Mutex; | ||
|
|
||
| #[tokio::test] | ||
| async fn borrow_and_repay() { |
There was a problem hiding this comment.
Having said that, fast and high-confidence unit tests for protocols are a little dream coming true. Can't believe it was so easy to do. So much time wasted in the past 😭
luckysori
force-pushed
the
release/0.2.0
branch
from
cf16679 to
03b76e4
Compare
luckysori
force-pushed
the
elements-consensus
branch
from
94dc5f3 to
c61b860
Compare
@thomaseizinger Can you give me some examples of negative tests you would like to see added to the library. I wouldn't tackle it here since we're just "refactoring" the tests, but I do want to make sure that we record an issue for this. |
For example, trying to repay a loan with the wrong amount, trying to repay a loan to the wrong address etc |
thomaseizinger
left a comment
thomaseizinger
left a comment
There was a problem hiding this comment.
Nice!
If this is not a feels-good PR, then I don't know what is :)
| let wallet = wallet.clone(); | ||
| |tx| async move { | ||
| let wallet = wallet.lock().unwrap(); | ||
| Ok(wallet.sign_inputs(tx)) | ||
| } |
There was a problem hiding this comment.
If you wouldn't move here, you should be able to get away with not cloning either which might allow you to get rid of Arc<Mutex>>.
There was a problem hiding this comment.
Great point. I was really hoping we could get rid of most of these.
It can be done except for those API calls which use the wallet twice: for coin selection and signing. These closures are probably going away soon anyway, so the situation should improve.
There was a problem hiding this comment.
We only need to use Arc<Mutex<Wallet>> once!
This means we no longer need `testcontainers`, ```elements-harness` or `elements-rpc`! And the tests are so much faster.
luckysori
force-pushed
the
elements-consensus
branch
from
913ffed to
a9d616b
Compare
2 participants
Fixes #21.