-
Notifications
You must be signed in to change notification settings - Fork 213
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
Return derivation path when answering isOurs
#2219
Conversation
94942f9
to
7056362
Compare
bc406dd
to
ac4222e
Compare
7056362
to
2ef0111
Compare
purpose :: Index 'Hardened 'PurposeK | ||
coinType :: Index 'Hardened 'CoinTypeK | ||
accountIx :: Index 'Hardened 'AccountK | ||
(purpose, coinType, accountIx) = undefined -- TODO: have this in the seq state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Forgot to change that one. Will do.
purpose :: Index 'Hardened 'PurposeK | ||
coinType :: Index 'Hardened 'CoinTypeK | ||
accountIx :: Index 'Hardened 'AccountK | ||
(purpose, coinType, accountIx) = undefined -- TODO: have this in the seq state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Forgot to change that one. Will do.
@@ -551,6 +555,8 @@ data SeqState (n :: NetworkDiscriminant) k = SeqState | |||
-- (cf: 'PendingIxs') | |||
, rewardAccountKey :: k 'AddressK XPub | |||
-- ^ Reward account public key associated with this wallet | |||
, derivationPrefix :: DerivationPrefix | |||
-- ^ Derivation path prefix from a root key up to the internal account |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two major changes in this PR (which then ripples to many other files). One of them is here. DerivationPrefix
is defined below and is simply a 3-tuple of the first three derivation indexes that leads to the account associated with that state.
This is necessary because we use seq-state for both Shelley and Icarus which have different purpose
levels. Also, this will build a nice foundation for the up-coming multi-account support by having the actual account index within the state itself too!
(_, Just addrIx) -> | ||
Just (purpose, coinType, accountIx, utxoInternal, addrIx) | ||
|
||
_ -> Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other main change is here, in the definition of isOurs
. Instead of returning a mere Bool
, we now return the full derivation path corresponding to that address.
isOurs addr st = | ||
(isJust path, maybe id (addDiscoveredAddress addr Used) path st) | ||
(path, maybe id (addDiscoveredAddress addr Used) path st) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For random addresses, this is trivially achieved because decrypting the path is exactly how we identify whether a random address is ours or not.
<*> fromText coinTypeT | ||
<*> fromText accountT | ||
_ -> | ||
Left $ TextDecodingError "expected exactly 3 derivation paths" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: add roundtrip tests to test these instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very fond of renaming initiative taken here - purposeCIP1852
, coinTypeAda
,.. all give more clarity. There is some more work to do (roundtrips, defined undefined, db migration support..etc) already mentioned. I wonder if some things (like purposeCI1852
) should not be (some day in future) relocated further to cardano-addresses and imported here in cardano-wallet
That's a good idea long-term indeed 👍 |
2ef0111
to
185d8fb
Compare
185d8fb
to
e6aecc7
Compare
0888d02
to
7e4a6a4
Compare
bors merge |
2219: Return derivation path when answering `isOurs` r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #2176 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 6c3aaea 📍 **change derivation path JSON serialization to be less verbose** And also aligned with other interfaces like cardano-addresses. - 1bba794 📍 **change 'isOurs' to return a derivation path instead of a boolean** This can then be used to figure out what are the derivation path of a bunch of addresses when returning raw coin-selections. Note that this commit builds but is so-to-speak unsound. We need to find a way to feed the purpose, coin type and account index down to the 'isOurs' function. The most logical place to do this is as part of the state. We can't use arbitrary constant here because both Icarus and Shelley use a SeqState, but have different purpose indexes. - 4b116de 📍 **store seq-state derivation prefix in the database.** That prefixes tells us which account corresponds to which state and also, which purpose so can distinguish between Icarus and Shelley wallets. This will require a database migration which I'll add in a later commit. - 185d8fb 📍 **define manual migrations for seq-state with regards to the derivation prefix** I've generated databases for Icarus and Shelley wallets from the latest master and, in a test now trying to open these database and observe that a) it is possible, b) there's a log line indicating that a migration has happened, c) the resulting prefix in each database is exactly what we expect it to be. # 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) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build failed:
#expected |
Damn. |
data ApiAddressDerivationSegment = ApiAddressDerivationSegment | ||
{ derivationIndex :: !ApiRelativeAddressIndex | ||
, derivationType :: !ApiAddressDerivationType | ||
data ApiDerivationSegment = ApiDerivationSegment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to use this data type in the Cardano.Wallet
, which must not use API types. Because this is similar to
newtype Index (derivationType :: DerivationType) (level :: Depth) = Index
{ getIndex :: Word32 }
which I can't use as in a homogenous List easily. ApiAddressDerivationSegment
would basically be non-typelevel version of Index
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or... we could define a toText instance for Index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using the DerivationIndex
from the Primitive.Types
instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't preserve the derivation type, don't we need that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When considering all indexes as a list (or path):
- the depth matters not because what the
Depth
captures is actually the position of the index in that list. - the derivationType is captured by representing indexes as plain Word32. The
Soft
/Hardened
notation is for easing human-readability but in the end, a soft index is simply a value < 2^31, whereas a "hardened" index is simply a value >= 2^31. Therefore, instead of representing indexes as derivationType + relative index within 0 and 2^31, we can represent them as just an index between 0 and 2^32, which is whatDerivationIndex
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should probably add these bits as comments.
648cab2
to
2755db9
Compare
This PR was included in a batch that timed out, it will be automatically retried |
2219: Return derivation path when answering `isOurs` r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #2176 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 6c3aaea 📍 **change derivation path JSON serialization to be less verbose** And also aligned with other interfaces like cardano-addresses. - 1bba794 📍 **change 'isOurs' to return a derivation path instead of a boolean** This can then be used to figure out what are the derivation path of a bunch of addresses when returning raw coin-selections. Note that this commit builds but is so-to-speak unsound. We need to find a way to feed the purpose, coin type and account index down to the 'isOurs' function. The most logical place to do this is as part of the state. We can't use arbitrary constant here because both Icarus and Shelley use a SeqState, but have different purpose indexes. - 4b116de 📍 **store seq-state derivation prefix in the database.** That prefixes tells us which account corresponds to which state and also, which purpose so can distinguish between Icarus and Shelley wallets. This will require a database migration which I'll add in a later commit. - 185d8fb 📍 **define manual migrations for seq-state with regards to the derivation prefix** I've generated databases for Icarus and Shelley wallets from the latest master and, in a test now trying to open these database and observe that a) it is possible, b) there's a log line indicating that a migration has happened, c) the resulting prefix in each database is exactly what we expect it to be. # 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) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build failed:
|
bors retry |
2219: Return derivation path when answering `isOurs` r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #2176 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 6c3aaea 📍 **change derivation path JSON serialization to be less verbose** And also aligned with other interfaces like cardano-addresses. - 1bba794 📍 **change 'isOurs' to return a derivation path instead of a boolean** This can then be used to figure out what are the derivation path of a bunch of addresses when returning raw coin-selections. Note that this commit builds but is so-to-speak unsound. We need to find a way to feed the purpose, coin type and account index down to the 'isOurs' function. The most logical place to do this is as part of the state. We can't use arbitrary constant here because both Icarus and Shelley use a SeqState, but have different purpose indexes. - 4b116de 📍 **store seq-state derivation prefix in the database.** That prefixes tells us which account corresponds to which state and also, which purpose so can distinguish between Icarus and Shelley wallets. This will require a database migration which I'll add in a later commit. - 185d8fb 📍 **define manual migrations for seq-state with regards to the derivation prefix** I've generated databases for Icarus and Shelley wallets from the latest master and, in a test now trying to open these database and observe that a) it is possible, b) there's a log line indicating that a migration has happened, c) the resulting prefix in each database is exactly what we expect it to be. # 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) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build failed:
|
bors retry |
2219: Return derivation path when answering `isOurs` r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #2176 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 6c3aaea 📍 **change derivation path JSON serialization to be less verbose** And also aligned with other interfaces like cardano-addresses. - 1bba794 📍 **change 'isOurs' to return a derivation path instead of a boolean** This can then be used to figure out what are the derivation path of a bunch of addresses when returning raw coin-selections. Note that this commit builds but is so-to-speak unsound. We need to find a way to feed the purpose, coin type and account index down to the 'isOurs' function. The most logical place to do this is as part of the state. We can't use arbitrary constant here because both Icarus and Shelley use a SeqState, but have different purpose indexes. - 4b116de 📍 **store seq-state derivation prefix in the database.** That prefixes tells us which account corresponds to which state and also, which purpose so can distinguish between Icarus and Shelley wallets. This will require a database migration which I'll add in a later commit. - 185d8fb 📍 **define manual migrations for seq-state with regards to the derivation prefix** I've generated databases for Icarus and Shelley wallets from the latest master and, in a test now trying to open these database and observe that a) it is possible, b) there's a log line indicating that a migration has happened, c) the resulting prefix in each database is exactly what we expect it to be. # 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) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Timed out.
|
And also aligned with other interfaces like cardano-addresses.
This can then be used to figure out what are the derivation path of a bunch of addresses when returning raw coin-selections. Note that this commit builds but is so-to-speak unsound. We need to find a way to feed the purpose, coin type and account index down to the 'isOurs' function. The most logical place to do this is as part of the state. We can't use arbitrary constant here because both Icarus and Shelley use a SeqState, but have different purpose indexes.
That prefixes tells us which account corresponds to which state and also, which purpose so can distinguish between Icarus and Shelley wallets. This will require a database migration which I'll add in a later commit.
… prefix I've generated databases for Icarus and Shelley wallets from the latest master and, in a test now trying to open these database and observe that a) it is possible, b) there's a log line indicating that a migration has happened, c) the resulting prefix in each database is exactly what we expect it to be.
…in resolved inputs.
Turns out that it is much easier and less error-prone to use a single opaque type 'DerivationIndex' to represent derivation path as homogeneous lists. Having 2 level of indirections in the API now makes thing more verbose and quite hard to grasp for a non-educated reader.
… about the derivation path.
2755db9
to
20e70b0
Compare
bors merge |
2219: Return derivation path when answering `isOurs` r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #2176 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - 6c3aaea 📍 **change derivation path JSON serialization to be less verbose** And also aligned with other interfaces like cardano-addresses. - 1bba794 📍 **change 'isOurs' to return a derivation path instead of a boolean** This can then be used to figure out what are the derivation path of a bunch of addresses when returning raw coin-selections. Note that this commit builds but is so-to-speak unsound. We need to find a way to feed the purpose, coin type and account index down to the 'isOurs' function. The most logical place to do this is as part of the state. We can't use arbitrary constant here because both Icarus and Shelley use a SeqState, but have different purpose indexes. - 4b116de 📍 **store seq-state derivation prefix in the database.** That prefixes tells us which account corresponds to which state and also, which purpose so can distinguish between Icarus and Shelley wallets. This will require a database migration which I'll add in a later commit. - 185d8fb 📍 **define manual migrations for seq-state with regards to the derivation prefix** I've generated databases for Icarus and Shelley wallets from the latest master and, in a test now trying to open these database and observe that a) it is possible, b) there's a log line indicating that a migration has happened, c) the resulting prefix in each database is exactly what we expect it to be. # 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) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build failed:
|
bors r+ |
Build succeeded: |
2235: Fix extra-source-files not containing files used by TH r=hasufell a=hasufell 2239: WIP: Bump version from 2020.9.30 to 2020.10.13 r=KtorZ a=jonathanknowles <!-- Short optional summary --> Compatible with [`jormungandr@v0.9.0`](https://github.com/input-output-hk/jormungandr/releases/tag/v0.9.0) and [`cardano-node@1.21.1`](https://github.com/input-output-hk/cardano-node/releases/tag/1.21.1). ## New Features - Adds the ability for users to select their own SMASH servers for stakepool listings. (#2214) - Adds transaction expiry slots for pending transactions. (#1879) ## Improvements - Adds a 100-wallet scenario to the latency benchmark. (#2223) - Adds an executable `shelley-test-cluster` which starts an integration test cluster that includes faucets. (#2178) - Extends `isOurs` to return the derivation path of an address. (#2219) ## Resolved Issues - Make pool garbage collection handle an unknown current epoch. (#2203) - Fixes incorrect mainnet network parameters returned from API. (#2226) ## Known Issues **_This section is a work in progress._** - Wallet restoration status reported incorrectly on mainnet. ([ADP-483](https://jira.iohk.io/browse/ADP-483)) ## Documentation <!-- A snapshot of the documentation at the time of releasing. --> Cardano (cardano-node) | ITN (Jörmungandr) --- | --- [API Documentation](https://input-output-hk.github.io/cardano-wallet/api/v2020-10-13) | [API Documentation](https://input-output-hk.github.io/cardano-wallet/api/v2020-10-13) [CLI Manual](https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-command-line-interface/f71ecb7ece3deaec5bf60f4a4beea1d3950e0d46) | [CLI Manual](https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-command-line-interface-jormungandr/f71ecb7ece3deaec5bf60f4a4beea1d3950e0d46) [Docker Manual](https://github.com/input-output-hk/cardano-wallet/wiki/Docker/f71ecb7ece3deaec5bf60f4a4beea1d3950e0d46) | [Docker Manual](https://github.com/input-output-hk/cardano-wallet/wiki/Docker-jormungandr/f71ecb7ece3deaec5bf60f4a4beea1d3950e0d46) ## Installation Instructions ### Cardano (cardano-node) 1. Install [`cardano-node@1.21.1`](https://github.com/input-output-hk/cardano-node/releases/tag/1.21.1). 2. Download the provided `cardano-wallet` for your platform, and uncompress it in a directory that is on your `$PATH`, e.g. `/usr/local/bin`. Or `%PATH%` on Windows. 4. Start `cardano-wallet --help` and see available parameters. #### Docker Pull from DockerHub and verify the version matches 2020.10.13. ``` $ docker pull inputoutput/cardano-wallet:2020.10.13 $ docker run --rm inputoutput/cardano-wallet:2020.10.13 version ``` ### ITN (jormungandr) 1. Install [`jormungandr@v0.9.0`](https://github.com/input-output-hk/jormungandr/releases/tag/v0.9.0). 2. Download the provided `cardano-wallet-jormungandr` for your platform, and uncompress it in a directory that is on your `$PATH`, e.g. `/usr/local/bin`. Or `%PATH%` on Windows. 3. (optional) Install the bash/zsh auto-completion script according to the [jormungandr cli manual](https://github.com/input-output-hk/cardano-wallet/wiki/Wallet-Command-Line-Interface/{{JORM_CLI_WIKI_COMMIT}}) 4. Start `cardano-wallet --help` and see available parameters. #### Docker Pull from DockerHub and verify the version matches 2020.10.13 ``` $ docker pull inputoutput/cardano-wallet:2020.10.13-jormungandr $ docker run --rm inputoutput/cardano-wallet:2020.10.13-jormungandr version ``` ## Signatures <!-- Signatures of people responsible for the release --> Name | Role | Approval --- | --- | ---: Matthias Benkort @KtorZ | Technical Team Lead | ⌛ Piotr Stachyra @piotr-iohk | QA Engineer | ⌛ Tatyana Valkevych @tatyanavych | Release Manager | ⌛ Co-authored-by: Julian Ospald <julian.ospald@iohk.io> Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Co-authored-by: IOHK <devops+stack-project@iohk.io>
Issue Number
#2176
Overview
6c3aaea
📍 change derivation path JSON serialization to be less verbose
And also aligned with other interfaces like cardano-addresses.
1bba794
📍 change 'isOurs' to return a derivation path instead of a boolean
This can then be used to figure out what are the derivation path of
a bunch of addresses when returning raw coin-selections.
Note that this commit builds but is so-to-speak unsound. We need to
find a way to feed the purpose, coin type and account index down to
the 'isOurs' function. The most logical place to do this is as part of
the state. We can't use arbitrary constant here because both Icarus
and Shelley use a SeqState, but have different purpose indexes.
4b116de
📍 store seq-state derivation prefix in the database.
That prefixes tells us which account corresponds to which state and also, which purpose so can distinguish between Icarus and Shelley wallets. This will require a database migration
which I'll add in a later commit.
185d8fb
📍 define manual migrations for seq-state with regards to the derivation prefix
I've generated databases for Icarus and Shelley wallets from the latest master and, in a test now trying to open these database and observe that a) it is possible, b) there's a log line indicating that a migration has happened, c) the resulting prefix in each database is exactly what we expect it to be.
Comments