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

Abstract class for pool datums and order datums #37

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

bhatt-deep
Copy link
Collaborator

@bhatt-deep bhatt-deep commented Jun 18, 2024

@theeldermillenial I have implemented the initial abstract classes and extended them in the Sundae DEX. Could you please take a quick look to confirm that this is the correct approach before I proceed further?

@theeldermillenial
Copy link
Collaborator

theeldermillenial commented Jun 19, 2024

Hey @bhatt-deep!

This looks mostly good. However, there are two things to modify:

  1. you will need to remove the dataclass decorator from OrderDatum
  2. you will need to change the OrderDatum(PlutusData) to OrderDatum(PlutusData, ABC), using from abc import ABC

Python has various issues with dataclass inheritance, one of which being dataclass attributes with default arguments. Basically, you can't really set default arguments on parent dataclasses because things go haywire. There is an option in Python 3.10 to assist with this, but that would mean that we only support Python 3.10+
https://stackoverflow.com/questions/69711886/python-dataclasses-inheritance-and-default-values

My recommendation is to make the parent class not a dataclass.

For the second part, I forget if this is still the case, but the abstractmethod used to not be enforced unless it was a method on an ABC class. You'll have to double check, but I'm pretty sure this is still the case.

@bhatt-deep bhatt-deep changed the base branch from main to dev June 19, 2024 21:38
@bhatt-deep bhatt-deep marked this pull request as ready for review June 19, 2024 21:38
@bhatt-deep
Copy link
Collaborator Author

Hey @theeldermillenial , Thank you for the feedback! I've made the necessary changes and extended the implementation across the remaining DEXs.

@theeldermillenial theeldermillenial merged commit c32fdd4 into dev Jun 21, 2024
1 check passed
@bhatt-deep bhatt-deep deleted the 9-abstract-class-for-pool-datums-low-cost branch July 10, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abstract class for order datums (low cost) Abstract class for pool datums (low cost)
2 participants