Skip to content
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

Balance for p2c #28

Closed
wants to merge 5 commits into from
Closed

Balance for p2c #28

wants to merge 5 commits into from

Conversation

Yamaguchi
Copy link
Contributor

Description

The PR adds an update to include coins transferred to a P2C address in the balance.

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
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@coveralls
Copy link

coveralls commented Aug 5, 2024

Pull Request Test Coverage Report for Build 10440638704

Details

  • 99 of 109 (90.83%) changed or added relevant lines in 4 files are covered.
  • 216 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.2%) to 81.772%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/wallet/src/wallet/mod.rs 66 76 86.84%
Files with Coverage Reduction New Missed Lines %
crates/chain/src/tx_graph.rs 1 86.63%
crates/wallet/src/wallet/utils.rs 1 92.55%
crates/chain/src/keychain/txout_index.rs 5 85.32%
crates/wallet/src/wallet/tx_builder.rs 23 85.07%
crates/wallet/src/wallet/signer.rs 31 63.1%
crates/wallet/src/wallet/coin_selection.rs 46 95.93%
crates/wallet/src/wallet/mod.rs 109 79.26%
Totals Coverage Status
Change from base Build 10314986241: -0.2%
Covered Lines: 10188
Relevant Lines: 12459

💛 - Coveralls

@Yamaguchi Yamaguchi force-pushed the balance_for_p2c branch 2 times, most recently from c0b8944 to 9e300a6 Compare August 5, 2024 05:39
@@ -2660,6 +2683,17 @@ impl Wallet {
&self.indexed_graph.index
}

/// Insert a descriptor with a keychain associated to it.
pub fn insert_descriptor(
Copy link
Collaborator

Choose a reason for hiding this comment

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

外からは呼ば差なそうなので pub をつけなくてもよいかと思いました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

別の指摘に従い、メソッド自体を削除しました。

@@ -296,6 +296,208 @@ pub fn get_funded_wallet_with_reissuable_and_change(

(wallet, tx1.txid(), color_id)
}

fn get_p2c_address(wallet: &mut Wallet, color_id: Option<ColorIdentifier>) -> Address {
let payment_base =
Copy link
Collaborator

Choose a reason for hiding this comment

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

wallet.store_contract() でwallet に追加するコントラクトのpayment_baseは、wallet で管理している秘密鍵に対応する公開鍵であることを想定していました。そうでないと、p2cアドレスあてのUTXOを送金できないので。

仕様には記載していませんでしたが、store_contract()でうけとった payment_base がウォレットで管理している鍵でない場合、エラーにするバリデーション処理を加えた方が安全な気がしました。
どう思われますか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

バリデーション処理を追加しました。

Copy link
Contributor Author

@Yamaguchi Yamaguchi Aug 18, 2024

Choose a reason for hiding this comment

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

f559dc8 でメソッドの位置を変更しました。

let (descriptor, _) =
Descriptor::<DescriptorPublicKey>::parse_descriptor(&self.secp, &descriptor_str)
.unwrap();
let _ = self.insert_descriptor(KeychainKind::External, descriptor);
Copy link
Collaborator

Choose a reason for hiding this comment

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

すでに wallet に KeychainKind::External な keychain が設定されている場合、新たに追加するのではなく、上書きしてしまうことになりませんか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

上記の理由からinsert_descriptorを使わず、indexerにpayment_baseと対応するp2c scriptの対応をマップ化して保持するように修正しました。

@rantan
Copy link
Collaborator

rantan commented Sep 2, 2024

#31 でマージしました。

@rantan rantan closed this Sep 2, 2024
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.

3 participants