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

Would TxId be better if it inherits HashDigest<SHA256>? #79

Closed
dahlia opened this issue Feb 16, 2019 · 7 comments
Closed

Would TxId be better if it inherits HashDigest<SHA256>? #79

dahlia opened this issue Feb 16, 2019 · 7 comments
Labels
good first issue Good for newcomers suggestion Suggestion or feature request

Comments

@dahlia
Copy link
Contributor

dahlia commented Feb 16, 2019

Since TxId is a SHA-256 digest, the most of things are in common with HashDigest<SHA256>. Would TxId be better to inherit HashDigest<256>? Or, rather, would it be fine even if we get rid of TxId at all and replace them with HashDigest<256>?

@dahlia dahlia added the suggestion Suggestion or feature request label Feb 16, 2019
@longfin
Copy link
Member

longfin commented Feb 23, 2019

I think considering TxId as HashDigest<SHA256> makes sense. but it also means that we allow to assign TxId to any HashDigest<SHA256> fields (e.g. Block.Hash). it can be quite fragile.

So, I suggest to make some other concrete type for these type like BlockHash to distinguish among them if we want to use inheritance.

@longfin longfin added the good first issue Good for newcomers label Feb 23, 2019
@longfin longfin added this to To do in comuka-20190223 via automation Feb 23, 2019
@dahlia
Copy link
Contributor Author

dahlia commented Feb 23, 2019

Having a distinct type for each BlockHash and TxId and they inheriting HashDigest<SHA256> seems good.

@longfin longfin added this to To do in comuka-20190309 Mar 8, 2019
@qria
Copy link
Contributor

qria commented Mar 9, 2019

I call dibs on this issue

@longfin longfin moved this from To do to In progress in comuka-20190309 Mar 9, 2019
@qria
Copy link
Contributor

qria commented Mar 9, 2019

HashDigest<T> was a struct not class so inheritance is not possible. what do?

@dahlia
Copy link
Contributor Author

dahlia commented Mar 9, 2019

@qria You're right. The idea in itself seems invalid.

@qria
Copy link
Contributor

qria commented Mar 9, 2019

I no longer am working on this until there's a new idea for this.

@longfin
Copy link
Member

longfin commented Mar 20, 2019

I'll close this issue until we have a new idea to address this issue.

@longfin longfin closed this as completed Mar 20, 2019
dahlia pushed a commit to dahlia/libplanet that referenced this issue Mar 9, 2021
limebell pushed a commit to limebell/libplanet that referenced this issue Jul 7, 2021
…-notification

Fix bug in ring notification condition.
OnedgeLee pushed a commit to OnedgeLee/libplanet that referenced this issue Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers suggestion Suggestion or feature request
Projects
No open projects
comuka-20190309
  
In progress
Development

No branches or pull requests

3 participants