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

Discovering in shared wallets #2675

Merged
merged 8 commits into from
May 28, 2021

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented May 26, 2021

Issue Number

adp-927

Overview

  • Adding the integration test showing the case
  • implement IsOurs and enable discovery
  • introduce hardened index guard outlawing improper ix in account when creating wallet
  • investigated another chain follower error for active shared wallet. It turn out there is very subtle error in sqlite handler when inserting the cosigner. Before that all cosigners were deleted which is incorrect, only cosigner row for a given credential and walid should be deleted. Otherwise, payment cosigners are deleted when delegation template is present and reading (selecting) is erroneous.

Comments

@paweljakubas paweljakubas self-assigned this May 26, 2021
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-927/discovering-in-shared-wallets branch 3 times, most recently from 05fba1f to f4a7a8d Compare May 28, 2021 11:31
@paweljakubas paweljakubas requested review from KtorZ, rvl and piotr-iohk May 28, 2021 11:32
@paweljakubas
Copy link
Contributor Author

@piotr-iohk could you confirm the two issues in the ticket are fixed now, please :-)

Copy link
Contributor

@rvl rvl 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.

, DerivationIndex $ getIndex coinType
, DerivationIndex $ getIndex accIx
, DerivationIndex $ getIndex utxoExternal
, DerivationIndex $ getIndex addrIx
Copy link
Contributor

Choose a reason for hiding this comment

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

This Index / DerivationIndex stuff ... not confusing at all!

wid = WalletId $ digest $ publicKey rootXPrv
pTemplate = scriptTemplateFromSelf (getRawKey accXPub) $ body ^. #paymentScriptTemplate
dTemplateM = scriptTemplateFromSelf (getRawKey accXPub) <$> body ^. #delegationScriptTemplate
wName = getApiT (body ^. #name)
accXPub = publicKey $ deriveAccountPrivateKey pwd rootXPrv accIx
accXPub = publicKey $ deriveAccountPrivateKey pwd rootXPrv (Index $ getDerivationIndex ix)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

@paweljakubas paweljakubas May 28, 2021

Choose a reason for hiding this comment

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

this is just code reshuffling allowing to use guardHardened above. No logic was changed

@piotr-iohk
Copy link
Contributor

@piotr-iohk could you confirm the two issues in the ticket are fixed now, please :-)

Looks good now! 👍 Both cannot be reproduced.

@paweljakubas
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request May 28, 2021
2675: Discovering in shared wallets r=paweljakubas a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->
adp-927

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Adding the integration test showing the case
- [x] implement IsOurs and enable discovery
- [x] introduce hardened index guard outlawing improper ix in account when creating wallet
- [x] investigated another chain follower error for active shared wallet. It turn out there is very subtle error in sqlite handler when inserting the cosigner. Before that all cosigners were deleted which is incorrect, only cosigner row for a given credential and walid should be deleted. Otherwise, payment cosigners are deleted when delegation template is present and reading (selecting) is erroneous.

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Pawel Jakubas <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 28, 2021

Build failed:

@Anviking
Copy link
Member

bors r+

iohk-bors bot added a commit that referenced this pull request May 28, 2021
2675: Discovering in shared wallets r=Anviking a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->
adp-927

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Adding the integration test showing the case
- [x] implement IsOurs and enable discovery
- [x] introduce hardened index guard outlawing improper ix in account when creating wallet
- [x] investigated another chain follower error for active shared wallet. It turn out there is very subtle error in sqlite handler when inserting the cosigner. Before that all cosigners were deleted which is incorrect, only cosigner row for a given credential and walid should be deleted. Otherwise, payment cosigners are deleted when delegation template is present and reading (selecting) is erroneous.

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Pawel Jakubas <[email protected]>
@paweljakubas paweljakubas force-pushed the paweljakubas/adp-927/discovering-in-shared-wallets branch from 2a152eb to bdd2b4b Compare May 28, 2021 17:45
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 28, 2021

Canceled.

@paweljakubas
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request May 28, 2021
2675: Discovering in shared wallets r=paweljakubas a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->
adp-927

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Adding the integration test showing the case
- [x] implement IsOurs and enable discovery
- [x] introduce hardened index guard outlawing improper ix in account when creating wallet
- [x] investigated another chain follower error for active shared wallet. It turn out there is very subtle error in sqlite handler when inserting the cosigner. Before that all cosigners were deleted which is incorrect, only cosigner row for a given credential and walid should be deleted. Otherwise, payment cosigners are deleted when delegation template is present and reading (selecting) is erroneous.

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Pawel Jakubas <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 28, 2021

Build failed:

cardano-wallet                 &gt;   src/Test/Integration/Scenario/API/Byron/Wallets.hs:187:59:
--
&nbsp; | cardano-wallet                 &gt;   1) API Specifications, BYRON_WALLETS, BYRON_RESTORE_01, GET_01, LIST_01 - Restore a wallet, ledger 21 words
&nbsp; | cardano-wallet                 &gt;        While verifying (Status {statusCode = 200, statusMessage = "OK"},Right (ApiByronWallet {id = ApiT {getApiT = WalletId {getWalletId = 7784a684aefeacf73e52ddd5746a1d0b7402347b}}, balance = ApiByronWalletBalance {available = Quantity {getQuantity = 0}, total = Quantity {getQuantity = 0}}, assets = ApiWalletAssetsBalance {available = ApiT {getApiT = TokenMap (fromList [])}, total = ApiT {getApiT = TokenMap (fromList [])}}, discovery = DiscoverySequential, name = ApiT {getApiT = WalletName {getWalletName = "Empty Byron Wallet"}}, passphrase = Just (ApiWalletPassphraseInfo {lastUpdatedAt = 2021-05-28 18:18:35.624318912 UTC}), state = ApiT {getApiT = Syncing (Quantity {getQuantity = Percentage {getPercentage = 1037 % 1250}})}, tip = ApiBlockReference {absoluteSlotNumber = ApiT {getApiT = SlotNo 2023}, slotId = ApiSlotId {epochNumber = ApiT {getApiT = EpochNo {unEpochNo = 40}}, slotNumber = ApiT {getApiT = SlotInEpoch {unSlotInEpoch = 23}}}, time = 2021-05-28 18:17:13.6 UTC, block = ApiBlockInfo {height = Quantity {getQuantity = 1000}}}}))
&nbsp; | cardano-wallet                 &gt;        Waited longer than 90s to resolve action: "wallet is available and ready".

#2565

@paweljakubas
Copy link
Contributor Author

spotted in integration testing:


cardano-wallet                 &gt; [cardano-wallet.network:Warning:2092] [2021-05-28 18:40:41.48 UTC] Connection lost with the node. Network.Socket.recvBuf: resource vanished (Connection reset by peer)
--



cardano-wallet                 &gt;
--



cardano-wallet                 &gt; Failures:
--



cardano-wallet                 &gt;
--



cardano-wallet                 &gt;   src/Test/Integration/Scenario/API/Byron/Wallets.hs:187:59:
--



cardano-wallet                 &gt;   1) API Specifications, BYRON_WALLETS, BYRON_RESTORE_01, GET_01, LIST_01 - Restore a wallet, ledger 21 words
--



cardano-wallet                 &gt;        While verifying (Status {statusCode = 200, statusMessage = "OK"},Right (ApiByronWallet {id = ApiT {getApiT = WalletId {getWalletId = 7784a684aefeacf73e52ddd5746a1d0b7402347b}}, balance = ApiByronWalletBalance {available = Quantity {getQuantity = 0}, total = Quantity {getQuantity = 0}}, assets = ApiWalletAssetsBalance {available = ApiT {getApiT = TokenMap (fromList [])}, total = ApiT {getApiT = TokenMap (fromList [])}}, discovery = DiscoverySequential, name = ApiT {getApiT = WalletName {getWalletName = "Empty Byron Wallet"}}, passphrase = Just (ApiWalletPassphraseInfo {lastUpdatedAt = 2021-05-28 18:18:35.624318912 UTC}), state = ApiT {getApiT = Syncing (Quantity {getQuantity = Percentage {getPercentage = 1037 % 1250}})}, tip = ApiBlockReference {absoluteSlotNumber = ApiT {getApiT = SlotNo 2023}, slotId = ApiSlotId {epochNumber = ApiT {getApiT = EpochNo {unEpochNo = 40}}, slotNumber = ApiT {getApiT = SlotInEpoch {unSlotInEpoch = 23}}}, time = 2021-05-28 18:17:13.6 UTC, block = ApiBlockInfo {height = Quantity {getQuantity = 1000}}}}))
--


cardano-wallet                 &gt;        Waited longer than 90s to resolve action: "wallet is available and ready".

@paweljakubas
Copy link
Contributor Author

bors retry

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 28, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 33629a0 into master May 28, 2021
@iohk-bors iohk-bors bot deleted the paweljakubas/adp-927/discovering-in-shared-wallets branch May 28, 2021 20:25
WilliamKingNoel-Bot pushed a commit that referenced this pull request May 28, 2021
2675: Discovering in shared wallets r=paweljakubas a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. -->
adp-927

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Adding the integration test showing the case
- [x] implement IsOurs and enable discovery
- [x] introduce hardened index guard outlawing improper ix in account when creating wallet
- [x] investigated another chain follower error for active shared wallet. It turn out there is very subtle error in sqlite handler when inserting the cosigner. Before that all cosigners were deleted which is incorrect, only cosigner row for a given credential and walid should be deleted. Otherwise, payment cosigners are deleted when delegation template is present and reading (selecting) is erroneous.

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->

Co-authored-by: Pawel Jakubas <[email protected]> 33629a0
@ghost

This comment has been minimized.

@ghost

This comment has been minimized.

@ghost

This comment has been minimized.

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.

4 participants