Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CO-325] API V1 Improvements - Part 2 #3461

Merged
merged 29 commits into from
Aug 24, 2018
Merged

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Aug 23, 2018

Description

  • The creation of mnemonic mustn't throw any runtime exception when provided with words outside of the BIP39 EN dictionnary
  • The API should provide a way to force-check NTP status when querying node-info
  • When running the new data-layer, The API should provide an endpoint to retrieve basic statistics on the UTxO distribution of a wallet
  • The code should rely on cryptonite rather than ed25519
  • The documentation should be complete and correct

TODO

  • Changelog incoming

Linked issue

[CO-325]

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Screenshots (if available)

@KtorZ KtorZ requested a review from edsko August 23, 2018 13:04
@KtorZ KtorZ self-assigned this Aug 23, 2018
@KtorZ
Copy link
Contributor Author

KtorZ commented Aug 23, 2018

@edsko

May you review the parts related to the new wallet. I introduced them mostly during rebased, adding commits as necessary.
Rest was already reviewed during development, when creating PR from feature branches to this one but it's still good to have a quick look at it.

@KtorZ KtorZ force-pushed the Squad1/CO-325/api-v1-improvements branch from 8d923d8 to 7387048 Compare August 23, 2018 13:16
fstRedKey = hexToBS "e2a1773a2a82d10c30890cbf84eccbdc1aaaee9204\
\96424d36e868039d9cb519"
sndRedKey = hexToBS "9cdabcec332abbc6fdf883ca5bf3a8afddca69bfea\
\c14c013304da88ac032fe6"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those examples were introduced rather recently and were "garbage" or more exactly, not valid Ed25519 keys. We used unsafe constructors in the past but cryptonite now requires us to use smart constructor ensuring key validity.

Hence, had to modify the corresponding golden test.

{"spec":{"avvmDistr":{"dtXbK7ASFLEa611AHt7SDk_48fVbNja-Tj7PgbE7tPE=":37343863242999412,"U4lgqRZyawnwXJ1NSpIrhbThGs_MFDRnPZUBm3qaUtI=":36524597913081152},"ftsSeed":"RTVTNGZTSDZldE5vdWlYZXpDeUVqS2MzdEc0amEwa0Y=","heavyDelegation":{"cae922c720ea93e5ead49b4652fb24caf4948e31eee378a4c4513f11":{"pskDelegatePk":"hPgRHpEzg2tVRuKD/U2unTt3PpxrVGVzdA==","pskOmega":68300481033,"pskCert":"bae5422af5405e3803154a4ad986da5d14cf624d6701c5c78a79ec73777f74e13973af83752114d9f18166085997fc81e432cab7fee99a275d8bf138ad04e103","pskIssuerPk":"ER6RM4NrVUbig/1Nrp07dz6ca1Rlc3Q="}},"blockVersionData":{"scriptVersion":999,"slotDuration":999,"maxBlockSize":999,"maxHeaderSize":999,"maxTxSize":999,"maxProposalSize":999,"mpcThd":9.9e-14,"heavyDelThd":9.9e-14,"updateVoteThd":9.9e-14,"updateProposalThd":9.9e-14,"updateImplicit":99,"softforkRule":{"initThd":9.9e-14,"minThd":9.9e-14,"thdDecrement":9.9e-14},"txFeePolicy":{"txSizeLinear":{"a":9.99e-7,"b":7.7e-8}},"unlockStakeEpoch":99},"protocolConstants":{"k":37,"protocolMagic":1783847074,"vssMaxTTL":1477558317,"vssMinTTL":744040476},"initializer":{"testBalance":{"poors":2448641325904532856,"richmen":14071205313513960321,"totalBalance":10953275486128625216,"richmenShare":4.2098713311249885,"useHDAddresses":true},"fakeAvvmBalance":{"count":17853231730478779264,"oneBalance":15087947214890024355},"avvmBalanceFactor":0.366832547637728,"useHeavyDlg":false,"seed":0}}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cf comment above.
Note that, this golden test output was obtained PRIOR to making the switch to cryptonite. It was generated using ed25519 and used after the switch to control that everything was still generated in the same way 👍

Copy link
Contributor

@edsko edsko left a comment

Choose a reason for hiding this comment

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

Only verified the changes that touch the new wallet kernel. Looks good. One minor change (we can be a bit more precise about possible errors), but that certainly doesn't need to block merging this.


forM (IxSet.toList accounts) $ \account -> do
hdAccId <- withExceptT GetUtxosWalletIdDecodingFailed $
fromAccountId wid (V1.accIndex account)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to use fromAccountId here. account :: HdAccount, so you can just use account ^. hdAccountId, so that you avoid this error condition.

fromAccountId wid (V1.accIndex account)

utxo <- withExceptT (GetUtxosErrorFromGetAccountError . GetAccountError . V1) $
exceptT $ Kernel.currentUtxo db hdAccId
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly no need for the potential error here. Can use account ^. cpUtxo.

@@ -156,6 +158,25 @@ instance Buildable DeleteWalletError where
build (DeleteWalletError kernelError) =
bprint ("DeleteWalletError " % build) kernelError

data GetUtxosError =
GetUtxosWalletIdDecodingFailed Text
| GetUtxosErrorFromGetAccountsError GetAccountsError
Copy link
Contributor

Choose a reason for hiding this comment

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

Only the first error condition here is needed (see comments below).

-> Either GetUtxosError [(V1.Account, Utxo)]
getWalletUtxos wid db = runExcept $ do
accounts <- withExceptT GetUtxosErrorFromGetAccountsError $
exceptT $ getAccounts wid db
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry missed this. This should not call getAccounts. That's using the V1 API to get access to the wallet, which is a lossy query. There's no need for that. Instead use fromRootId to translate the WalletId to a HdRootId (that may fail with a decoding error, which is why the first error condition is still needed), and then use accountsByRootId from Cardano.Wallet.Kernel.DB.Read to get the IxSet of accounts.

@KtorZ KtorZ force-pushed the Squad1/CO-325/api-v1-improvements branch from 7387048 to 31ceae8 Compare August 23, 2018 15:16
paweljakubas and others added 22 commits August 24, 2018 12:08
…of hs-ed25519 normalized extended secret key plus removing hs-ed25519 from default nix
[CO-325] Generate valid RedeemPublicKey for test rather than hard-coded ones

Also, had to replace the golden test. Note that the patch on
`Test/Pos/Core/Json.hs` was also applied on develop (before removing the
ed25519 library), and the golden test generated from there AND THEN,
ported to this branch to actually make sure that it runs as expected.

[CO-325] Add missing encodedSizeExpr from Bi Redeem* instances

Without this, tests will fail because they can't determine what should be the size of the given encoded output.
This revision includes recent API changes done to the BIP39 module, as well as a fix to prevent the
library from throwing upon receiving an word not present in the BIP39 english dictionnary
We now have thiner granularity upon variety of errors. Functions don't
return `Maybe` anymore but good and informative `Either`.
This gives a bit more context to understand where the errors come from if they randomly pop-out in the wild
…E doc

The doc was incomplete in some parts or not up-to-date, leading to confusion for newcomers
Not sure how this ended-up here. It was remove WEEKS ago, and I am pretty sure this the third time I am doing
it... I am actually afraid that some other parts of the doc were touched as well during wrong conflict resolutions
KtorZ and others added 6 commits August 24, 2018 12:08
Kernel level implementation of WalletLayer is added, Legacy is not
supported. The implementation is used in Handlers of V1 API. Refactoring
from Integer to Word64 is done
- We do not expose type internals and provide one smart constructor for that purpose (computeUtxoStatistics)
- Same for the 'BoundType' which should actually be part of the API if we intend to use it, making it an opaque
  type with exposed constructors allow for easy extension and maintainability
- Reviewed a bit errors to make constructors a bit less specified in favor of constructor with arg
- Added 'BoundType' as 'boundType' to the JSON representation
- Made Aeson & Swagger imports explicit!
This is more semantically correct and type-safe than taking a raw list of 'Word64'. This way, we also get documentation
for free simply by looking at the function signature and also makes calls for callers simpler (provided they have a list
of available utxos, but why would they call the function if they hadn't? :) )
@KtorZ KtorZ force-pushed the Squad1/CO-325/api-v1-improvements branch from 555e721 to 97ca2cc Compare August 24, 2018 10:08
@KtorZ KtorZ changed the base branch from develop-new to develop August 24, 2018 12:03
@KtorZ KtorZ force-pushed the Squad1/CO-325/api-v1-improvements branch from 97ca2cc to 8c2b82b Compare August 24, 2018 12:04
@KtorZ KtorZ merged commit ea2a5b6 into develop Aug 24, 2018
@KtorZ
Copy link
Contributor Author

KtorZ commented Aug 24, 2018

Merging without AppVeyor ... as usual.

@KtorZ KtorZ deleted the Squad1/CO-325/api-v1-improvements branch August 24, 2018 12:29
@KtorZ KtorZ mentioned this pull request Sep 17, 2018
11 tasks
KtorZ added a commit that referenced this pull request Nov 9, 2018
…provements

[CO-325] API V1 Improvements - Part 2
KtorZ added a commit to input-output-hk/cardano-wallet-legacy that referenced this pull request Nov 9, 2018
…hk/Squad1/CO-325/api-v1-improvements

[CO-325] API V1 Improvements - Part 2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants