Fix weeder 2.x config and remove weeds #2765

rvl · 2021-07-19T09:59:47Z

Issue Number

This could fix #2695, I really hope it does.

Overview

I was looking into #2695, running weeder locally to see what it could be doing.

But it turns out that Weeder has been doing nothing at all since upgrading weeder to 2.1.3, which happened when we upgraded to ghc-8.10.4.

It needed to have a ghc option added to generate .hie files, which I didn't set before. If Weeder can't find .hie files, it just reports zero weeds (ocharles/weeder#19).

So I have:

Fixed the Weeder config for Stack and Cabal.
Removed weeds which would have been introduced while weeder wasn't working.
Removed weeds found by Weeder 2.x, which were never found by Weeder 1.0.9.

Comments

 49 files changed, 109 insertions(+), 1681 deletions(-)

Anviking

👍 My, that's a lot of changes (skimmed them)

Anviking · 2021-07-19T14:35:20Z

lib/core/test/unit/Cardano/Wallet/Api/Malformed.hs

-            , ("{\"transaction\": 1020344, \"passphase\": \"Secure Passphrase\"}", "Error in $.transaction: parsing 'Base64 ByteString failed, expected String, but encountered Number")
-            , ("{\"transaction\": { \"body\": 1020344 }, \"passphase\": \"Secure Passphrase\"}", "Error in $.transaction: parsing 'Base64 ByteString failed, expected String, but encountered Object")
+            , ("{\"transaction\": \"lah\", \"passphrase\": \"Secure Passphrase\"}", "Error in $.transaction: Parse error. Expecting Base64-encoded format.")
+            , ("{\"transaction\": 1020344, \"passphrase\": \"Secure Passphrase\"}", "Error in $.transaction: parsing 'Base64 ByteString failed, expected String, but encountered Number")


This mis-spelling wasn't intentional?

It was a mistake - found when changing to strictRecordTypeOptions.

Anviking · 2021-07-19T14:38:01Z

lib/shelley/src/Cardano/Wallet/Shelley/Compatibility.hs

-        -- some non-trivial changes. Not fixing this right now is also a
-        -- possibility.
-        , parentHeaderHash = W.Hash "parentHeaderHash - unused in Shelley"
+        , parentHeaderHash = W.Hash ""


Removing parentHeaderHash would be nice, but is this "parentHeaderHash - unused in Shelley" to "" change safe? It couldn't affect compatibility with existing DBs somehow?

No, I think we only do logic with the BlockHeader after converting it to a Point CardanoBlock, so this should be fine.

I tried removing parentHeaderHash but then found that it is sort of used in Cardano.Wallet.restoreBlocks !

Perhaps you could try removing parentHeaderHash? To keep DB compatibility, just leave the field there, but don't use it.

I suspect it might make sense to do with/after #2750 if we can replace BlockHeader with Point CardanoBlock and be able to persist the Origin point in a nicer way. Currently I think rollbacks to Origin might break:

G 0 1 <-- Wallet has checkpoints at genesis, slot 0 and slot 1 Node asks wallet to rollback to G Wallet believes genesis has slotNo 0: G 0 Node asks wallet to re-apply block in slot 0: G 0 0 <- Wrong!

Although...

This invariant is nice to have, particularly as we lack great chain-following testing
https://github.com/input-output-hk/cardano-wallet/blob/8d1b692fec0c77e488a724dba97c9fa51ab9f4e5/lib/core/src/Cardano/Wallet.hs#L888-L893

I am a bit concerned about how to handle backward compatibility regarding how Origin is encoded https://github.com/input-output-hk/cardano-wallet/blob/914668d4e40db6a0b999869c4c4f5222fa6884d0/lib/shelley/src/Cardano/Wallet/Shelley/Compatibility.hs#L392-L398

@hamishmack

2766: Misc dev env fixes r=rvl a=rvl ### Issue Number Found while working on #2765. ### Overview This is a mixed bag of small tweaks. - ~Adds `.dir-locals.el` with conservative presets for Emacs users.~ - Add tracing of job names to Hydra evaluation (thanks for the tip @hamishmack) - Delete some unneeded code in `shell.nix` (thanks for the tip @jbgi @nrdxp) - Change `scripts/gen-hie.sh` to make symlinks if `hie.yaml` and `.ghci` don't exist. - `.gitignore` and `.editorconfig` tweaks. Co-authored-by: Rodney Lorrimar <[email protected]>

jonathanknowles

Hi @rvl.

It's good to see all these weeds removed!

However, I think this PR also does a couple of other things:

Makes some behavioural changes to tests
Renames some functions

These behavioural changes are bundled together into a single commit with the weeding changes.

Request: Would it be possible to move the behavioural changes (and function renamings) into a separate PR?

This would make it easier to give these non-weeding changes the scrutiny they deserve, I think.

Example of a function renaming:

- shrinkCoinSmall :: Coin -> [Coin]
- shrinkCoinSmall (Coin c) = Coin <$> shrink c
+ shrinkCoin :: Coin -> [Coin]
+ shrinkCoin (Coin c) = Coin <$> shrink c

Example of a behavioural change:

 instance Arbitrary Coin where
     arbitrary = genCoinSmallPositive
-    shrink = shrinkCoinSmallPositive
+    shrink = shrinkCoin

Before: The shrinker was limited to positive, non-zero values.
After: The shrinker can now produce zero values.

It might be the case that generating zero values is fine here. But I think we should make sure, just in case. And there are other similar changes like this, within a relatively large commit that otherwise just deletes weeds.

It would be great if we could move these changes into a separate PR and give them a good review of their own!

rvl · 2021-07-20T05:21:03Z

OK done @jonathanknowles - here is your PR: #2768.

lib/core/test/unit/Cardano/Wallet/Primitive/Types/TokenMapSpec.hs

jonathanknowles · 2021-07-20T06:20:17Z

lib/core/src/Cardano/Wallet/Primitive/Types/UTxO.hs

-        return (Nothing, UTxO utxo)
-    | otherwise = do
-        ix <- randomRIO (0, toEnum (Map.size utxo - 1))
-        return (Just $ Map.elemAt ix utxo, UTxO $ Map.deleteAt ix utxo)


Ah yes! This is a vestige of the old coin selection algorithm. :)

jonathanknowles

Hi @rvl

This looks mostly good, though I think there is still one behavioural change w.r.t. shrinking that got left behind (see review comment).

Could you also move this to the other PR (#2768), so it can be considered together with the other changes?

Many thanks!

The shrinker for `Arbitrary TokenQuantity` should allow zero values. See: #2765 (review)

rvl · 2021-07-20T08:11:19Z

bors r+

2765: Fix weeder 2.x config and remove weeds r=rvl a=rvl ### Issue Number This could fix #2695, I really hope it does. ### Overview I was looking into #2695, running `weeder` locally to see what it could be doing. But it turns out that Weeder has been doing nothing at all since upgrading `weeder` to 2.1.3, which happened when we upgraded to ghc-8.10.4. It needed to have a ghc option added to generate `.hie` files, which I didn't set before. If Weeder can't find `.hie` files, it just reports zero weeds (ocharles/weeder#19). So I have: 1. Fixed the Weeder config for Stack and Cabal. 2. Removed weeds which would have been introduced while weeder wasn't working. 3. Removed weeds found by Weeder 2.x, which were never found by Weeder 1.0.9. ### Comments ``` 49 files changed, 109 insertions(+), 1681 deletions(-) ``` Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2021-07-20T08:43:29Z

Build failed:

buildkite/cardano-wallet

weeder
--
  | incompatible hie file: /build/cardano-wallet/lib/shelley/.stack-work/dist/x86_64-linux/Cabal-3.2.1.0/build/unit/unit-tmp/Cardano/Wallet/Shelley/LaunchSpec.hie
  | expected .hie file version 8104 but got 8105
  | weeder must be built with the same GHC version as the project it is used on
  | error: Command exited with code 1!

#expected - wrong weeder build used

Makes the function definition clearer as well.

The shrinker for `Arbitrary TokenQuantity` should allow zero values. See: #2765 (review)

Anviking · 2021-07-20T12:20:34Z

bors r+

iohk-bors · 2021-07-20T14:38:57Z

Build succeeded:

rvl self-assigned this Jul 19, 2021

rvl mentioned this pull request Jul 19, 2021

Misc dev env fixes #2766

Merged

rvl force-pushed the rvl/2695/weeder branch 2 times, most recently from 79ed59d to 6d765f1 Compare July 19, 2021 13:38

Anviking approved these changes Jul 19, 2021

View reviewed changes

jonathanknowles reviewed Jul 20, 2021

View reviewed changes

rvl force-pushed the rvl/2695/weeder branch 2 times, most recently from 541b31b to 4aeb220 Compare July 20, 2021 05:17

jonathanknowles reviewed Jul 20, 2021

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/Types/TokenMapSpec.hs Outdated Show resolved Hide resolved

jonathanknowles reviewed Jul 20, 2021

View reviewed changes

jonathanknowles added a commit that referenced this pull request Jul 20, 2021

Restore shrinking behaviour for TokenQuantity in TokenMapSpec.

83e51d5

The shrinker for `Arbitrary TokenQuantity` should allow zero values. See: #2765 (review)

jonathanknowles added a commit that referenced this pull request Jul 20, 2021

Restore shrinking behaviour for TokenQuantity in TokenMapSpec.

cb2c01d

The shrinker for `Arbitrary TokenQuantity` should allow zero values. See: #2765 (review)

jonathanknowles force-pushed the rvl/2695/weeder branch from 83e51d5 to cb2c01d Compare July 20, 2021 07:22

jonathanknowles self-requested a review July 20, 2021 07:29

jonathanknowles approved these changes Jul 20, 2021

View reviewed changes

rvl and others added 9 commits July 20, 2021 17:51

Fix config for weeder >= 2.0

066d107

Remove weeds

c3be768

Remove obvious TokenBundle.Gen weeds

cfce60c

Remove Data.Text.Class.splitAtLastOccurrence weed

71063d9

Remove Cardano.Launcher.withBackendProcess weed

e8a2537

Add FIXME on AddressDerivation.SharedKey.toNetworkTag

8c59a56

Makes the function definition clearer as well.

Use Cardano.Wallet.Api.Types.strictRecordTypeOptions

7eda703

Restore shrinking behaviour for TokenQuantity in TokenMapSpec.

983f393

The shrinker for `Arbitrary TokenQuantity` should allow zero values. See: #2765 (review)

Buildkite: Use correct weeder version

3f6fe49

rvl force-pushed the rvl/2695/weeder branch from cb2c01d to 3f6fe49 Compare July 20, 2021 09:51

iohk-bors bot merged commit f8455ff into master Jul 20, 2021

iohk-bors bot deleted the rvl/2695/weeder branch July 20, 2021 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix weeder 2.x config and remove weeds #2765

Fix weeder 2.x config and remove weeds #2765

rvl commented Jul 19, 2021

Anviking left a comment

Anviking Jul 19, 2021

rvl Jul 19, 2021

Anviking Jul 19, 2021

Anviking Jul 19, 2021

rvl Jul 19, 2021

Anviking Jul 19, 2021

rvl Jul 19, 2021

Anviking Jul 19, 2021

jonathanknowles left a comment •

edited

Loading

rvl commented Jul 20, 2021

jonathanknowles Jul 20, 2021

jonathanknowles left a comment

rvl commented Jul 20, 2021

iohk-bors bot commented Jul 20, 2021 •

edited by rvl

Loading

Anviking commented Jul 20, 2021

iohk-bors bot commented Jul 20, 2021

Fix weeder 2.x config and remove weeds #2765

Fix weeder 2.x config and remove weeds #2765

Conversation

rvl commented Jul 19, 2021

Issue Number

Overview

Comments

Anviking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles left a comment • edited Loading

Choose a reason for hiding this comment

rvl commented Jul 20, 2021

Choose a reason for hiding this comment

jonathanknowles left a comment

Choose a reason for hiding this comment

rvl commented Jul 20, 2021

iohk-bors bot commented Jul 20, 2021 • edited by rvl Loading

Anviking commented Jul 20, 2021

iohk-bors bot commented Jul 20, 2021

jonathanknowles left a comment •

edited

Loading

iohk-bors bot commented Jul 20, 2021 •

edited by rvl

Loading