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.

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

Merged
merged 6 commits into from
May 3, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented May 3, 2019

Issue Number

Overview

  • I have created a new package 'http-bridge' which contains the implementation details related to the bridge and the Byron era. This achieves the full decoupling between the wallet core logic and the underlying network binary representation and formats!!

Comments

@KtorZ KtorZ requested a review from akegalj May 3, 2019 13:59
@KtorZ KtorZ self-assigned this May 3, 2019
@@ -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
Copy link
Member Author

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
Copy link
Member Author

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))
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Member Author

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'
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@KtorZ KtorZ merged commit 05ec32d into master May 3, 2019
@KtorZ KtorZ deleted the KtorZ/extract-http-bridge branch May 3, 2019 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants