-
Notifications
You must be signed in to change notification settings - Fork 188
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
Collection Malleability #7114
base: feature/malleability
Are you sure you want to change the base?
Collection Malleability #7114
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/malleability #7114 +/- ##
=====================================================
Coverage 41.23% 41.23%
=====================================================
Files 2168 2168
Lines 190257 190216 -41
=====================================================
- Hits 78448 78432 -16
+ Misses 105228 105206 -22
+ Partials 6581 6578 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is only used in tests. It represents an anti-pattern for non-test code, as it creates a partially constructed entity (missing signatures etc.). Replaced with direct struct instantiation for test use cases
used in too many places to deal with here, but can be removed toward end of malleability workstream
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.
Looks great, thanks for documentation updates
// Fingerprint returns the canonical, unique byte representation for the TransactionBody. | ||
// As RLP encoding logic for TransactionBody is over-ridden by EncodeRLP below, this is | ||
// equivalent to directly RLP encoding the TransactionBody. | ||
// This public function is retained primarily for backward compatibility. |
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 am a bit confused about backward compatibility aspect. I don't see it being used, maybe we can just remove it?
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'm thinking of third-parties using flow-go
as a dependency - this seemed like a function fairly likely to be used in that context and I wanted to avoid breaking changes. Just a hunch though, and they should be using flow-go-sdk
...
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.
Looks good to me!
Co-authored-by: Uliana Andrukhiv <[email protected]>
This PR cleans up
Fingerprint
definitions forTransactionBody
,Collection
, andcluster.Payload
. It also adds tests demonstrating non-malleability ofCollection
.Fingerprint
This PR removes the
Fingerprint
method fromTransactionBody
,Collection
, andcluster.Payload
, in favour of defining anEncodeRLP
method forTransactionBody
. In contrast withFingerprint
, theEncodeRLP
method is used for encoding the transaction even when it appears in nested structures. This way, we don't need to implementFingerprint
for every data structure that includes aTransactionBody
somewhere.Fingerprint
methods except forTransactionBody
, which now delegates toEncodeRLP
, since it is more likely to be used externally and was a public functionOther Changes
CollectionFromTransactions
tounittest
package as it is only used in testsCollection.Guarantee()
method, as it is only used in tests and constitutes an anti-pattern of creating a partially constructed object.CollectionList
, which was unusedRef: #6721