Additions structure field tx changed to txs #972

Shourya742 · 2023-05-05T16:23:10Z

Description

This PR solves the issue : #954

Notes to the reviewers

Additions structure field "tx" is changed to "txs" and relevant changes are made to all files using it.

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

Bugfixes:

I'm linking the issue being fixed by this PR

crates/chain/src/tx_graph.rs

vladimirfomene

Can you squash all the commits into one?

Shourya742 · 2023-05-08T18:09:47Z

Can you squash all the commits into one?

I am facing some issues while squashing the commit. Don't know why there are merge conflicts. I am quite new to this and it's a bit confusing. Please help me with this. Sorry for the inconvenience.

vladimirfomene · 2023-05-09T19:21:41Z

@Shourya742 I think you merged your changes into upstream/master instead of rebasing your changes on it. That is what is stopping you from squashing these changes. You have to do an interactive rebase where you get rid of that merge commit. You can read more about rebase here: https://www.youtube.com/watch?v=7Mh259hfxJg. Let me know how it goes.

Shourya742 · 2023-05-12T02:06:37Z

Thank you @vladimirfomene for your assistance, I gained a lot of knowledge from this experience. Could you kindly review the branch and provide any suggestions for changes? I appreciate your help!

vladimirfomene

Review ACK 0026848. You might want to read this article: https://danielabrozzoni.com/posts/contributing_to_oss/. It will help you on your journey as you strive to make progress as a contributor.

Shourya742 · 2023-05-12T13:33:45Z

@vladimirfomene Thanks, will go through the article.

Shourya742 · 2023-05-24T13:48:00Z

@vladimirfomene Hello, may I inquire if there is any particular issue concerning the pull request? I'm curious as to why it has not yet been merged.

vladimirfomene · 2023-05-24T15:05:32Z

@evanlinjin, can you look into merging this?

danielabrozzoni

utACK 0026848 - looks good to me

danielabrozzoni · 2023-05-24T15:53:52Z

I can't merge as your commits aren't signed, can you fix? https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

evanlinjin · 2023-05-24T16:03:20Z

Thank you for the work but please don't merge this. Will result in a lot of pain and conflicts imo.

I suggest we wait until #976 is merged, or we base this work on top of #976.

However, this is such a minor change that can be handled with rust-analyzer in one go anyway.

Shourya742 · 2023-05-25T03:56:04Z

Thank you, @evanlinjin, for your assistance. There are no problems at the moment. I will patiently wait until #976 is merged. Once it is, I will update the commit and request a review. I am hopeful that my first contribution will be merged. In the meantime, I will explore other issues.

This is to make it easier for chain source crates to formulate updates.

This allows us to skip adding an extra input to `Wallet::insert_tx`. Also remove redundant logic.

There are a number of improvements that can be done, but it is in a decent state to be usable. Possible improvements: * Remove requirement to retry obtaining ALL data after reorg is detected. Transactions can be anchored to a lower block (not block tip), and an `assume_final_depth` value can be used. * The logic to finalize an update with confirmation time can be improved during reorgs to not require returning an error.

This is the equivalent of `keychain_tracker_example_cli` that works with the redesigned structures.

This is a version of `keychain_tracker_electrum` that uses the redesigned structures instead.

This corresponds to `keychain::KeychainChangeSet` but for the redesigned structures with `LocalChain`. This structure is now used in `Wallet` as well as the examples.

@danielabrozzoni

As per reviews by @danielabrozzoni and @LLFourn

* `ElectrumUpdate::missing_full_txs` now returns a `Vec<Txid>` so we don't keep a reference to the passed-in `graph`. * `ElectrumUpdate::finalize*` methods now takes in `missing` txids instead of `full_txs`. `Client::batch_transaction_get` is called within the methods. Other changes: * `wallet::ChangeSet` is now made public externally. This is required as a wallet db should implement `PersistBackend<wallet::ChangeSet>`.

All associated examples are also updated.

Other changes: * The `async-https` feature of `bdk_esplora` is no longer default. * Rename `ObservedAs` to `ChainPosition`. * Set temporary MSRV to 1.60.0 to compile all workspace members will all features.

evanlinjin · 2023-05-26T11:47:41Z

@Shourya742 because #976 is basically ready to be merged (waiting on reviews by others). I suggest that we rebase this work on top of that PR. I can cherry-pick that commit to be part of #976.

Remember that we need a signed commit.

Shourya742 · 2023-05-26T18:28:07Z

Hello @evanlinjin, I have rebased my local issue branch onto your wallet_redesign branch. It is possible that I may have misunderstood the instruction to "rebase this work on top of that PR." Please find attached the current commit graph for your reference. Moreover, I have resolved all merge conflicts encountered during the rebase and have made necessary changes to electrum_ext.rs.

Commit Graph

evanlinjin · 2023-05-27T03:17:19Z

@Shourya742 where did you push it? I don't have access to your local branches.

The commit I see in this PR is 0026848 which is different to that in the image.

Shourya742 · 2023-05-27T04:57:27Z

@evanlinjin No, I haven't pushed the code yet. I was just hoping to ensure that I handled the rebase correctly or if I may have made any errors. If everything is in order, I would proceed with pushing the code.

evanlinjin · 2023-05-27T05:10:32Z

@Shourya742 the commit history looks good. Please make sure tests pass, clippy is happy and everything compiles before pushing.

Shourya742 · 2023-05-27T05:33:04Z

@evanlinjin I have completed running the tests, and Clippy appears to be good. I have also signed the commit. Kindly review it, please.

crates/chain/src/chain_graph.rs

Changed txout to txouts field in Addition Structure Some more changes Changes made after rebasing to wallet_redesign branch Removed chain_graphs

Shourya742 · 2023-05-27T06:41:13Z

@evanlinjin have a look...

evanlinjin · 2023-05-27T09:13:21Z

I've cherry-picked the commit to #976 and gave it a proper commit message. In the future, please remember to clean up your commit messages.

The new commit: 965cbe6

This PR is no longer needed.

nondiremanuel added the BDK 1.0.0 label May 6, 2023

vladimirfomene reviewed May 6, 2023

View reviewed changes

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

notmandatory removed the BDK 1.0.0 label May 6, 2023

vladimirfomene suggested changes May 7, 2023

View reviewed changes

Shourya742 force-pushed the issue-954 branch from 41bdc6b to 0026848 Compare May 12, 2023 02:02

vladimirfomene approved these changes May 12, 2023

View reviewed changes

notmandatory added this to the 1.0.0-alpha.1 milestone May 23, 2023

danielabrozzoni approved these changes May 24, 2023

View reviewed changes

evanlinjin and others added 13 commits May 26, 2023 19:37

[wallet_redesign] Update Wallet with redesigned structures

6fe3909

[wallet_redesign] Move the majority of Update to bdk_chain

2af1a24

This is to make it easier for chain source crates to formulate updates.

Add last_seen to the the ConfirmationTime::Unconfirmed variant

24cc32b

This allows us to skip adding an extra input to `Wallet::insert_tx`. Also remove redundant logic.

[examples_redesign] Introduce example_cli package

c9170ef

This is the equivalent of `keychain_tracker_example_cli` that works with the redesigned structures.

[examples_redesign] Implemented example_electrum

008b30a

This is a version of `keychain_tracker_electrum` that uses the redesigned structures instead.

[example-cli] simplify new address logic

3fe2fc3

Introduce keychain::LocalChangeSet

095af5b

This corresponds to `keychain::KeychainChangeSet` but for the redesigned structures with `LocalChain`. This structure is now used in `Wallet` as well as the examples.

Various tweaks to code arrangement and documentation

6046f6f

As per reviews by @danielabrozzoni and @LLFourn

Update the wallet_electrum example

1d3c602

Implement redesigned versions of EsploraExt and EsploraAsyncExt

0c0ea12

All associated examples are also updated.

[chain_redesign] Remove old structures

b275687

Other changes: * The `async-https` feature of `bdk_esplora` is no longer default. * Rename `ObservedAs` to `ChainPosition`. * Set temporary MSRV to 1.60.0 to compile all workspace members will all features.

Shourya742 force-pushed the issue-954 branch from 0026848 to 5614823 Compare May 27, 2023 05:10

Shourya742 force-pushed the issue-954 branch from 5614823 to 928ff1e Compare May 27, 2023 05:30

evanlinjin requested changes May 27, 2023

View reviewed changes

crates/chain/src/chain_graph.rs Outdated Show resolved Hide resolved

Shourya742 force-pushed the issue-954 branch from 928ff1e to ba6f00e Compare May 27, 2023 06:36

Additions structure field tx changed to txs

6a29207

Changed txout to txouts field in Addition Structure Some more changes Changes made after rebasing to wallet_redesign branch Removed chain_graphs

Shourya742 force-pushed the issue-954 branch from ba6f00e to 6a29207 Compare May 27, 2023 06:38

evanlinjin closed this May 27, 2023

notmandatory assigned Shourya742 Jul 4, 2023

notmandatory removed this from the 1.0.0-alpha.1 milestone Jul 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additions structure field tx changed to txs #972

Additions structure field tx changed to txs #972

Shourya742 commented May 5, 2023 •

edited by danielabrozzoni

vladimirfomene left a comment

Shourya742 commented May 8, 2023

vladimirfomene commented May 9, 2023

Shourya742 commented May 12, 2023

vladimirfomene left a comment •

edited

Shourya742 commented May 12, 2023 •

edited

Shourya742 commented May 24, 2023

vladimirfomene commented May 24, 2023

danielabrozzoni left a comment

danielabrozzoni commented May 24, 2023 •

edited

evanlinjin commented May 24, 2023 •

edited

Shourya742 commented May 25, 2023

evanlinjin commented May 26, 2023 •

edited

Shourya742 commented May 26, 2023 •

edited

evanlinjin commented May 27, 2023

Shourya742 commented May 27, 2023

evanlinjin commented May 27, 2023

Shourya742 commented May 27, 2023

Shourya742 commented May 27, 2023

evanlinjin commented May 27, 2023

Additions structure field tx changed to txs #972

Additions structure field tx changed to txs #972

Conversation

Shourya742 commented May 5, 2023 • edited by danielabrozzoni

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

Bugfixes:

vladimirfomene left a comment

Choose a reason for hiding this comment

Shourya742 commented May 8, 2023

vladimirfomene commented May 9, 2023

Shourya742 commented May 12, 2023

vladimirfomene left a comment • edited

Choose a reason for hiding this comment

Shourya742 commented May 12, 2023 • edited

Shourya742 commented May 24, 2023

vladimirfomene commented May 24, 2023

danielabrozzoni left a comment

Choose a reason for hiding this comment

danielabrozzoni commented May 24, 2023 • edited

evanlinjin commented May 24, 2023 • edited

Shourya742 commented May 25, 2023

evanlinjin commented May 26, 2023 • edited

Shourya742 commented May 26, 2023 • edited

evanlinjin commented May 27, 2023

Shourya742 commented May 27, 2023

evanlinjin commented May 27, 2023

Shourya742 commented May 27, 2023

Shourya742 commented May 27, 2023

evanlinjin commented May 27, 2023

Shourya742 commented May 5, 2023 •

edited by danielabrozzoni

vladimirfomene left a comment •

edited

Shourya742 commented May 12, 2023 •

edited

danielabrozzoni commented May 24, 2023 •

edited

evanlinjin commented May 24, 2023 •

edited

evanlinjin commented May 26, 2023 •

edited

Shourya742 commented May 26, 2023 •

edited