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.

Sign up for GitHub

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

Jump to bottom

[ADP-416] Allow multiple well-formed values in "/Cardano.Wallet.Api/"… #2085

Merged
merged 2 commits into from
Sep 1, 2020
Merged

[ADP-416] Allow multiple well-formed values in "/Cardano.Wallet.Api/"… #2085

merged 2 commits into from
Sep 1, 2020

Conversation

hasufell
Copy link
Contributor

Related: #2055 (comment)

Test log shows that it tries both variants of pool id encodings when combined with #2055

@hasufell hasufell requested a review from KtorZ August 27, 2020 18:04
hasufell
hasufell commented Aug 27, 2020
View reviewed changes
lib/core/test/unit/Cardano/Wallet/Api/Malformed.hs
@@ -150,7 +150,9 @@ instance Malformed (PathParam ApiTxId) where
msg = "Invalid tx hash: expecting a hex-encoded value that is 32 bytes in length."

instance Wellformed (PathParam ApiPoolId) where
wellformed = PathParam $ T.replicate 64 "0"
wellformed = PathParam <$>
[ T.replicate 64 "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.

This is where the bech32 wellformed would go

@KtorZ KtorZ added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Aug 27, 2020
jonathanknowles
jonathanknowles requested changes Aug 31, 2020
View reviewed changes
Copy link
Member

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far. 👍

Just a couple of requested changes, and a few suggestions. (See comments!)

lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated Show resolved Hide resolved
lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Show resolved Hide resolved
@hasufell hasufell force-pushed the hasufell/ADP-416/multi-well-formed branch from ee2942c to a9d46b3 Compare August 31, 2020 08:43
@hasufell hasufell force-pushed the hasufell/ADP-416/multi-well-formed branch from 659566f to 67e684a Compare August 31, 2020 08:57
jonathanknowles
jonathanknowles approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Hi @hasufell

This looks good to me. Thanks for addressing the previous feedback! (See comments.) 👍

I suspect @KtorZ will also have an opinion on this approach (judging by #2055 (comment)), so it would be good to get confirmation from him, if you can!

lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated
Comment on lines 465 to 466


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

https://github.com/input-output-hk/adrestia/wiki/Coding-Standards#use-only-a-single-blank-line-between-top-level-definitions

KtorZ
KtorZ approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

Some minor remarks about consistency and readability that I fixed in a commit.
Nice work dealing with this, hope you didn't caught a headache 😬

lib/core/test/unit/Cardano/Wallet/ApiSpec.hs
@@ -218,31 +218,38 @@ instance (GenericApiSpec a, GenericApiSpec b) => GenericApiSpec (a :<|> b) where
instance GenericApiSpec Request where
gSpec _ _ = pure ()

instance GenericApiSpec a => GenericApiSpec [a] where
gSpec xs toSpec = foldr (\x y -> gSpec x toSpec >> y) (pure ()) xs
Copy link
Member

Choose a reason for hiding this comment

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

I mistakenly expected a foldl here but no, foldr is right because of the composition of actions done as such 👍

lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated
toSpec . SomeTest (Proxy @a) =<< traverseLeft runIO tests
let tests :: [(IO [Request], ExpectedError)]
tests = first toRequest <$> malformed @(BodyParam a)
toSpec . SomeTest (Proxy @a) . insideOut =<< traverseLeft runIO tests
Copy link
Member

Choose a reason for hiding this comment

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

👍

lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated
SomeTest (Proxy @h) (first toRequest <$> malformed @(Header h ct))
gSpec toRequest toSpec = do
let tests = fmap (\(xs, e) -> fmap (,e) xs)
(first toRequest <$> malformed @(Header h ct))
Copy link
Member

Choose a reason for hiding this comment

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

I find the insideOut version with the list comprehension much more readable

lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated
where
t = wellformed :: PathParam t
t = wellformed :: [PathParam t]
Copy link
Member

Choose a reason for hiding this comment

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

Would be better to stick to a consistent naming like ts before. These things are actually the same thing so they ought to have the same name.

@KtorZ
Copy link
Member

KtorZ commented Sep 1, 2020

bors r+

@hasufell
Copy link
Contributor Author

hasufell commented Sep 1, 2020

Nice work dealing with this, hope you didn't caught a headache grimacing

I did... and I completely forgot everything I did 🤣 so don't ask me anything about this code.

hasufell
hasufell commented Sep 1, 2020
View reviewed changes
lib/core/test/unit/Cardano/Wallet/ApiSpec.hs
@@ -565,6 +579,9 @@ instance
-- Helpers
--

distributeFirst :: [([x], y)] -> [(x, y)]
Copy link
Contributor Author

@hasufell hasufell Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks... I wasn't able to come up with a sensible name.

The downside is that this won't inline in something like fmap distributeFirst xs. But I don't think there's anything performance critical about it.

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 1, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 8b0c493 into cardano-foundation:master Sep 1, 2020
hasufell added a commit to hasufell/cardano-wallet that referenced this pull request Sep 2, 2020
@hasufell
Add bech32 Pool id variant to Wellformed test
33b07cd 
Wrt cardano-foundation#2085 and cardano-foundation#2055
hasufell added a commit to hasufell/cardano-wallet that referenced this pull request Sep 2, 2020
@hasufell
Add bech32 Pool id variant to Wellformed test
04f5148 
Wrt cardano-foundation#2085 and cardano-foundation#2055
@hasufell hasufell mentioned this pull request Sep 2, 2020
Merged
iohk-bors bot added a commit that referenced this pull request Sep 3, 2020
@iohk-bors @hasufell
Merge #2106
5adcae2 
2106: Add bech32 Pool id variant to Wellformed test r=KtorZ a=hasufell

Wrt #2085 and #2055

Co-authored-by: Julian Ospald <julian.ospald@iohk.io>
iohk-bors bot added a commit that referenced this pull request Sep 5, 2020
@iohk-bors @hasufell
Merge #2105 #2106
2c20415 
2105: Add test case for importing invalid addresses for Byron r=Anviking a=hasufell

#2011 

2106: Add bech32 Pool id variant to Wellformed test r=Anviking a=hasufell

Wrt #2085 and #2055

Co-authored-by: Julian Ospald <julian.ospald@iohk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanknowles jonathanknowles jonathanknowles approved these changes

@KtorZ KtorZ KtorZ approved these changes

Assignees

@hasufell hasufell

Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hasufell @KtorZ @jonathanknowles