Skip to content

Expose latest_checkpoint in Wallet implementation#761

Merged
thunderbiscuit merged 1 commit intobitcoindevkit:masterfrom
ItoroD:expose-latest-checkpoint
May 16, 2025
Merged

Expose latest_checkpoint in Wallet implementation#761
thunderbiscuit merged 1 commit intobitcoindevkit:masterfrom
ItoroD:expose-latest-checkpoint

Conversation

@ItoroD
Copy link
Copy Markdown
Collaborator

@ItoroD ItoroD commented May 12, 2025

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature

@ItoroD ItoroD force-pushed the expose-latest-checkpoint branch from b5d81bf to ed9a8bf Compare May 12, 2025 16:11
@ItoroD ItoroD changed the title feat: Expose latest_checkpoint in Wallet implementation Expose latest_checkpoint in Wallet implementation May 12, 2025
@ItoroD
Copy link
Copy Markdown
Collaborator Author

ItoroD commented May 12, 2025

Not sure if the println added to test LiveTransactionTest.kt is necessary. I figured it might be worth putting it there to test that the lastest_checkpoint addition works. Creating a whole new test for just that seemed overkill. Let me know it I should take it out.

Copy link
Copy Markdown
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Looks good! One little comment and I think it's good to go.

Comment thread bdk-ffi/src/types.rs Outdated
fn from(block_id: BdkBlockId) -> Self {
BlockId {
height: block_id.height,
hash: block_id.hash.to_string(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here I think you can use the new BlockHash type if you strong-type the hash field in the BlockId type.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

When I attempt to update hash type to BlockHash, I get an error at compile.

the trait uniffi::Lower<UniFfiTag> is not implemented for bitcoin::BlockHash

Assuming BlockHash works, I will need to deal with the other places using hash of BlockId type I have identified just 1 place so far. (FYI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok it works. Issue was that I did not add the Arc around the BlockHash. My bad!

@ItoroD ItoroD force-pushed the expose-latest-checkpoint branch from ed9a8bf to 3cf40d7 Compare May 15, 2025 11:42
@ItoroD
Copy link
Copy Markdown
Collaborator Author

ItoroD commented May 15, 2025

I noticed a test is expecting String for BlockHash (hence the failing test even). Its the binding test. I will update it.

Copy link
Copy Markdown
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Just one last little thing!

As for the tests, you can actually remove the blockId variables from the tests. They were intended to test the bindings generation for bitcoin-ffi and bdk-ffi (as per the comments), but now we only use bdk-ffi so you can just keep the network enum and the tests will do what we need them to do. Just to be clear this is what I mean:

// Swift
let network = Network.testnet
// Kotlin
val network = Network.TESTNET
# Python
class TestBdk(unittest.TestCase):

    def test_some_enum(self):
        network = Network.TESTNET

if __name__=='__main__':
    unittest.main()

Comment thread bdk-ffi/src/types.rs Outdated
fn from(block_id: BdkBlockId) -> Self {
BlockId {
height: block_id.height,
hash: Arc::from(BlockHash(block_id.hash)),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think here you should use Arc::new() instead of Arc::from().

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I noticed. Thanks!

@ItoroD ItoroD force-pushed the expose-latest-checkpoint branch from 3cf40d7 to 34aa59f Compare May 15, 2025 22:08
@ItoroD ItoroD requested a review from thunderbiscuit May 16, 2025 05:37
Copy link
Copy Markdown
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 34aa59f.

@thunderbiscuit thunderbiscuit merged commit 34aa59f into bitcoindevkit:master May 16, 2025
26 checks passed
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