Skip to content

Test listlockunspent #332

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

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

Conversation

jamillambert
Copy link
Collaborator

Testing listlockunspent uses lockunspent and listunspent. This highlighted a couple of issues:

  1. lockunspent needs a txid as an input to lock a specific transaction, I am not sure anything is done if nothing is passed?
  2. When something is returned from listunspent there is an error due to a missing field account. This is listed in the help as deprecated and returns "" for the default account, but in the test it is not returned at all.

Address these and add a test:

  1. Update lockunspent client macro to take a txid.
  2. Make account field in listunspentitem an option.
  3. Add a test for listlockunspent.

Comment on lines 635 to 636
let txid = utxo.txid.parse::<bitcoin::Txid>().expect("parse txid");
let vout = utxo.vout as u32;
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use utxo.into_model() ?

Copy link
Member

Choose a reason for hiding this comment

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

i.e., model::ListLockUnspentItem has these fields.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed.

Copy link
Member

Choose a reason for hiding this comment

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

In fd50399 the wallet__lock_unspent() test still has this same pattern in it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I forgot I had that bit in this patch. Done now and swapped the patch that changes account to an option to be first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A small aside, because I switched to jj and have not got a fixed workflow yet: I rearranged the commits and did the edits without rebasing onto the updated master like I used to with git.
As a reviewer do you care either way, assuming no merge conflicts with the new master?

let model: Result<mtype::ListLockUnspent, ListLockUnspentItemError> = json.into_model();
let model = model.unwrap();

assert!(model.0.iter().any(|o| o.txid.to_string() == utxo.txid && o.vout as i64 == utxo.vout));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need to allocate a string here. In general we only want to allocate if totally necessary. (This is a bit of geek thing, we always tend to optimize for speed even in test code.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed by addressing the previous comment.

@jamillambert
Copy link
Collaborator Author

Addressed the comments.

@tcharding
Copy link
Member

Reviewed ce1d980

When an unspent item is returned it does not contain the deprecated 
field `account`.  Make the field an Option.
Testing `listlockunspent` requires `lockunspent` to lock a specified 
transaction.

Change the client macro to take a `txid` and update the test.
`listlockunspent` was untested.

Add a test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants