-
Notifications
You must be signed in to change notification settings - Fork 152
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
UntypedBlock
& UntypedTransaction
#1978
Conversation
574a4e8
to
c347e62
Compare
UntypedBlock
& UntypedTransaction
UntypedBlock
& UntypedTransaction
a50a1a8
to
f911fdb
Compare
{ | ||
/// <summary> | ||
/// Similar to <see cref="ITxMetadata"/> except that it has <see cref="TxId"/> as well. | ||
/// Note that this does not contain actions or signature. | ||
/// </summary> | ||
public interface ITxExcerpt : ITxMetadata |
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.
Can we clarify it's use a bit more?
As I understand it, seems like this is to "group" UntypedTransaction
and Transaction<T>
for validating nonces. In any case, the relation between ITxExcerpt
and ITxMetadata
should mirror that of IBlockExcerpt
and IBlockMetadata
if possible. 😕
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 defined the term excerpt as a set of metadata functioning as a proof without complete data (usually lacking its body like actions and transactions). That's why IBlockExcerpt
has Hash
, and ITxExcerpt
has Id
.
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 defined the term excerpt as a set of metadata
I see excerpts are some sort of metadata. On the other hand, ITxMetadata
and IBlockMetadata
have analogous specific uses. I'm still confused as to what this is "proving" really and if ITxExcerpt : ITxMetadata
is necessary.
- If the term excerpt is to extend metadata interfaces with additional proof data, than inheritance is granted.
- If the term excerpt is to be used for naming an auxiliary
interface
with minimal amount of information than there shouldn't be an inheritance relation.
I'm suggesting that either we have IBlockExcerpt : IBlockMetadata
and ITxExcerpt : ITxMetadata
(this is the former of the above) or not have inheritance relations between them (the latter of the above). The split design seems confusing.
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.
Ah, I see.
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.
Not to pester, but is this being worked on? I'm done with skimming reviewing the code, but I wasn't sure about your last comment. 😶
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 tried to make ITxMetadata
to inherit nothing, but it wasn't that easy as TxExcerptExtensions.ValidateTxNonces()
used several properties of ITxMetadata
. Instead, I'm going to rename its name later.
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.
👌
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.
[changelog skip]
[changelog skip]
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Closes #1974, and partially addresses #1164 (comment).
This introduces
UntypedBlock
andUntypedTransaction
under a new project named Libplanet.Node.