-
Notifications
You must be signed in to change notification settings - Fork 217
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
Extract Http-Bridge specifics into a dedicated package #212
Conversation
@@ -64,7 +64,7 @@ jobs: | |||
name: "Compiling" | |||
script: | |||
- mkdir -p ~/.local/bin | |||
- travis_retry curl -L -o cardano-node-simple.tar.gz https://raw.githubusercontent.com/input-output-hk/cardano-wallet/master/lib/core/test/data/cardano-node-simple/cardano-node-simple-3.0.1.tar.gz | |||
- travis_retry curl -L -o cardano-node-simple.tar.gz https://raw.githubusercontent.com/input-output-hk/cardano-wallet/KtorZ/extract-http-bridge/lib/http-bridge/test/data/cardano-node-simple/cardano-node-simple-3.0.1.tar.gz |
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.
Again, temporary, will change after merged.
@@ -93,14 +93,14 @@ jobs: | |||
script: | |||
- export NETWORK=mainnet | |||
- tar xzf $STACK_WORK_CACHE | |||
- stack --no-terminal test cardano-wallet-core:unit | |||
- stack --no-terminal test cardano-wallet-http-bridge:unit |
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.
So in CI, we have two jobs running tests. One for testnet which run everything including integration tests. And another one for mainnet, to run a couple of unit tests with a different protocol magic. Yet, we only pattern-match on the protocol magic (and actually, have access to it) in the bridge code so there's no need to re-run the core tests on mainnet.
return $ ApiT . SignedTx $ bs | ||
bs <- (base64 <$> (p .: "signedTx")) | ||
>>= either fail return | ||
tx <- pure (CBOR.deserialiseFromBytes decodeSignedTx (BL.fromStrict bs)) |
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.
is this change compatible with "old data" - if tx witness is stored on a blockchain
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.
aha - in fact I see it didn't change at all. You have just moved some code bits here
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.
Indeed. The code is the same, but now done within the http-bridge
package (rather than in the core), because it uses some cbor-specific encoding. So, before that, the core was sending a 'SignedTx ByteString, where the
ByteStringwas supposedly the serialized tx, and it now sends a
(Tx, [TxWitness])` and the network layer takes care of serializing it. This keeps the network layer agnostic to the binary format.
chgs'' = drop 1 chgs | ||
inps' = if length inps > 1 then drop 1 inps else inps | ||
outs' = if length outs > 1 then drop 1 outs else outs | ||
chgs' = drop 1 chgs | ||
in | ||
filter (\s -> s /= sel && isValidSelection s) | ||
[ CoinSelection inps' outs' chgs' |
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 see that we have reduced generator scope of CoinSelection - hope coverage is still big enough to test relevant bits
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've only adjusted the shrinker here to be less aggressive and therefore a bit more readable. The more aggressive version is especially needed for the property that has been relocated.
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.
yes I see it here 088c8cc#diff-80e66ce4975ed1ec822ac0aa910b4553R775
Issue Number
Overview
Comments