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

Do some header decoding #245

Merged
merged 1 commit into from
May 15, 2019
Merged

Do some header decoding #245

merged 1 commit into from
May 15, 2019

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented May 8, 2019 •
edited
Loading

Issue Number

#218

Overview

  • getBlockHeader :: Get BlockHeader with unit test
  • Scaffold for later (getBlock, Message, ...)

Comments

@Anviking Anviking self-assigned this May 8, 2019
@Anviking Anviking force-pushed the anviking/218/decode-headers branch 10 times, most recently from cb6ff72 to a5db312 Compare May 14, 2019 13:14
@Anviking Anviking changed the base branch from anviking/218/jörmungandr-blocks to master May 14, 2019 13:14
@Anviking Anviking force-pushed the anviking/218/decode-headers branch from a5db312 to 6ce2341 Compare May 14, 2019 13:21
Anviking
Anviking commented May 14, 2019
View reviewed changes
lib/jormungandr/src/Cardano/Wallet/Binary/Jormungandr.hs
data ConfigParam = ConfigParam
deriving Show

{-# ANN getBlockHeader ("HLint: ignore Use <$>" :: String) #-}
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't want to use applicative syntax. Wanted to keep the binary format separate from the BlockHeader constructor and the ordering of its arguments.

lib/jormungandr/src/Cardano/Wallet/Binary/Jormungandr.hs
--
-- Header
--
data Block = Block BlockHeader [Message]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a different Block from Cardano.Wallet.Primitive.Types.Bock. Not sure how this will play out, but thinking we keep it here and can convert to Cardano.Wallet.Primitive.Types.Bock in another place as required.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm..... Okay. we will do some mapping then most probably. Although it'd be nice if this could just happen in the decoder itself.

Copy link
Member Author

@Anviking Anviking May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In that case we will ignore everything but Transaction messages for now?

E.g. the initial protocol/config parameters, update proposals, and others?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. As we'll most probably make our definition of Block evolve when and if we take the others into consideration.

@Anviking Anviking marked this pull request as ready for review May 14, 2019 13:22
@Anviking Anviking requested a review from KtorZ May 14, 2019 13:23
@Anviking Anviking force-pushed the anviking/218/decode-headers branch 2 times, most recently from a73dd6b to a31b823 Compare May 14, 2019 13:57
@KtorZ KtorZ mentioned this pull request May 14, 2019
Closed
5 tasks
@Anviking Anviking force-pushed the anviking/218/decode-headers branch from a31b823 to 5935a96 Compare May 14, 2019 19:25
@Anviking
Copy link
Member Author

Thanks @KtorZ! Addressed.

Should I have removed the stub for Block and Message in this pr or is it fine?

@Anviking Anviking force-pushed the anviking/218/decode-headers branch 2 times, most recently from 7c397d4 to bacb66f Compare May 14, 2019 23:34
@KtorZ
Copy link
Member

KtorZ commented May 15, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request May 15, 2019
@iohk-bors @Anviking
Merge #245
5a74e17 
245: Do some header decoding r=KtorZ a=Anviking

# Issue Number

#218 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- `getBlockHeader :: Get BlockHeader` with unit test
- Scaffold for later (`getBlock`, `Message`, ...)

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <[email protected]>
@KtorZ KtorZ force-pushed the anviking/218/decode-headers branch from bacb66f to 493f76c Compare May 15, 2019 08:21
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 15, 2019

Canceled

@KtorZ KtorZ merged commit 4db16b6 into master May 15, 2019
@KtorZ KtorZ deleted the anviking/218/decode-headers branch May 15, 2019 10:44
akegalj
akegalj reviewed May 22, 2019
View reviewed changes
Copy link
Contributor

@akegalj akegalj left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akegalj akegalj akegalj left review comments

@KtorZ KtorZ KtorZ approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Anviking @KtorZ @akegalj