-
Notifications
You must be signed in to change notification settings - Fork 502
Use Data backed ledger types for COOP #7235
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
base: master
Are you sure you want to change the base?
Conversation
| Memory: 3_572_506 | ||
| Term Size: 4_267 | ||
| Flat Size: 10_061 | ||
| CPU: 1_189_413_270 |
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.
🤯
| (con unit ()) | ||
| An error has occurred: | ||
| The machine terminated because of an error, either from a built-in function or from an explicit use of 'error'. | ||
| Caused by: error |
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.
No clue why this is. No script logic was changed. 🫠
|
ana-pantilie
left a 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 think you need to try to write the code where you convert between Data and SOP without the conversions. There are many instances of this in the PR, I didn't comment on all of them. Let me know if you need any help with that.
| <> "Must have a specified CurrencySymbol in the Value" | ||
| Just tokenNameMap -> | ||
| case AssocMap.toList tokenNameMap of | ||
| case AssocMap.toSOPList tokenNameMap of |
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.
This may be one of the reasons there is a performance decrease. Data.AssocMap.toSOPList has to traverse the whole map, while the AssocMap.toList doesn't have any runtime cost. Can you avoid using toSOPList here?
| Value . AssocMap.safeFromSOPList . Map.toList . (AssocMap.safeFromSOPList . Map.toList <$>) $ | ||
| Map.unionsWith | ||
| (Map.unionWith (+)) | ||
| ( [ Map.singleton cs (Map.singleton tn q) | ||
| | (cs, tokens) <- AssocMap.toList . getValue $ v | ||
| , (tn, q) <- AssocMap.toList tokens | ||
| | (cs, tokens) <- AssocMap.toSOPList . getValue $ v | ||
| , (tn, q) <- AssocMap.toSOPList tokens |
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.
Again, Data <-> SOP conversions are expensive. Can this be avoided here? You won't be able to use the list comprehension, unfortunately, but there should be a way to write this code using builtins. Let me know if you need help with this and we can take a look together.
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.
It doesn't really matter on this module because this module generates script context as Data. So all the inefficient computation is only on the haskell side.
|
I can't really figure out why some of the scripts(fsMpMinting, fsMpBurning mainly) have slowed down. All that's changed from fsMp script is changing SOP list functions to Builtin function, but no clue.. I'm not gonna burn too much time here. |
| (con unit ()) | ||
| An error has occurred: | ||
| The machine terminated because of an error, either from a built-in function or from an explicit use of 'error'. | ||
| Caused by: error |
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 like you've introduced a bug somewhere
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.
yep, can't really figure this out either. Only thing changed is using builtin list utilities instead of SOP one
Changes COOP to use Data backed ledger types.
Some scripts have reduced cost, some increased(!!). This is probably because how scripts are written originally; original scripts pass around many un-deconstructed data and deconstruct those when needed, perhaps this is very expensive here.
@zliu41 Turns out
RecordWildCardsworks perfectly fine with datatypes defined withasData. I was just being delusional.