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

Remove TxId Coupling To Binary Format #207

Merged
merged 3 commits into from
May 3, 2019
Merged

Remove TxId Coupling To Binary Format #207

merged 3 commits into from
May 3, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented May 2, 2019

Issue Number

Overview

  • I have introduced a new abstraction 'TxId' in order to decouple the core code from the binary representation. The actual representation is now selected when the wallet server is instantiated which allows us in the long run, to switch easily from one binary format to another only in the cli code.

Comments

Note that the Primitive.Signing module explicitly reference the 'HttpBridge' binary representation because we actually need more work here. As for the address representation, those requires another abstraction for the on-chain representation of these data.

@KtorZ KtorZ requested a review from akegalj May 2, 2019 16:15
@KtorZ KtorZ self-assigned this May 2, 2019
@KtorZ KtorZ force-pushed the KtorZ/review-folder-structure branch from 72b967e to dfce9b7 Compare May 2, 2019 16:37
@KtorZ KtorZ force-pushed the KtorZ/txid-coupling branch from 2f78fcd to 6408c6f Compare May 2, 2019 18:43
@KtorZ KtorZ changed the base branch from KtorZ/review-folder-structure to master May 2, 2019 18:45
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

txId :: Tx -> Hash "Tx"
txId = blake2b256 . encodeTx
instance TxId HttpBridge where
txId = blake2b256 . encodeTx
Copy link
Contributor

Choose a reason for hiding this comment

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

so my understanding is that txId implemented by HttpBridge (ie, rust code) is something not strictly specified and is likeley to change in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

its pretty late so I might be wrong.

I see two scenarios that might be motivation for introduction of more flexible txId:

  1. One day we might need to change representation of txId and we will need to switch. When we switch , we will probably have to support old and new version
  2. Maybe there is a need to support multiple ways how to compute txId at the same time

for 2) it totally makes sense to implement it - if there is a need to support multiple versions at the same time. If 1) is expected to happen rather soonish, then added complexity makes sense.

(this is my attempt to figure out why we needed this feature - the explanations above, if true, are enough for me)

Copy link
Member Author

Choose a reason for hiding this comment

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

@akegalj So yes, sorry it's coming a bit of nowhere (more on Monday in our planning meeting); We will, in the very near future, start integrating with the Rust nodes which use a different binary format for blocks, addresses and transactions. With also in some cases, different hashing algorithms. On the other hand, we do want to keep open this current implementation with Byron and the bridge for the transition period and also because, that's what the Haskell nodes will also be using initially (with maybe some small variations, so we may likely end up with 3 different instances of the above!)

@KtorZ KtorZ force-pushed the KtorZ/txid-coupling branch from 6408c6f to 7f82ec1 Compare May 3, 2019 06:31
@KtorZ KtorZ merged commit 1dd70e7 into master May 3, 2019
@KtorZ KtorZ deleted the KtorZ/txid-coupling branch May 3, 2019 06:58
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