-
Notifications
You must be signed in to change notification settings - Fork 35
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
Datum and redeemer insertion order #130
Conversation
FYI this is #86 . I'm not sure about using LinkedHashMap vs a Vec since LinkedHashMap has led to some issues in the past. @rooooooooob can you sign off on this if this is okay? |
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.
@SebastienGllmt this won't address #86 fully though as PlutusMap
is still a BTreeMap
. Although I guess that is more for deserialization and such. This wouldn't cause an issue with the order conflicting in the built txs right? Since those are used as lists? Does this address the reason why in #86 they had changed it? Was it for building txs?
Could you refresh my mind on what issues LinkedHashMap
caused? (bug-wise). I can only remember it causing issues due to BTreeMap
automatically being canonical form so if we needed to serialize canonically (e.g. tx hashes) switching this out would break it. Or maybe traits not being implemented for it e.g. the JSON schema ones or CBOR ones which is why we have a wrapper around it in the latest version of codegen to implement those. Neither of those should be relevant here so I think it's okay, assuming this addresses the root reason why in the fork they changed it.
rust/src/builders/witness_builder.rs
Outdated
@@ -337,7 +342,9 @@ impl TransactionWitnessSetBuilder { | |||
} | |||
|
|||
pub fn get_redeemer(&self) -> Redeemers { | |||
Redeemers(self.redeemers.clone().into_values().collect()) | |||
Redeemers(self.redeemers.clone().iter().map(|i|{ |
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 don't think this clone is necessary anymore since iter()
should give you refs to iterate over which you are cloning in that map anyway. Same with the one above in get_plutus_datum
.
Yeah, I think the issues with LinkedHashMap were:
|
It should fix the preserving encodings though, not cause it. That's why we've been using it in places. If this covers the root reason why the fork changed it I'm fine with this change, although the minor nit would be those clones are unnecessary afaik. A full replacement to |
Cool I'll change the clones |
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.
LGTM
Ensure datum and redeemer order is maintained when building a transaction.