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

Reconstructable deterministic constrids #272

Merged
merged 8 commits into from
Oct 23, 2023

Conversation

nielstron
Copy link
Contributor

This resolves #271

Please reach out if there are any remaining questions as to the necessity or implementation of this.

@nielstron nielstron force-pushed the feat/reconstructible_det_constrids branch from a5b2245 to 955bab8 Compare October 4, 2023 21:06
Copy link
Collaborator

@cffls cffls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small conflict otherwise lgtm.

Comment on lines 451 to 487
def id_map(cls, skip_constructor=False):
"""
Constructs a unique representation of a PlutusData type definition.
Intended for automatic constructor generation.
"""
if cls == bytes:
return "bytes"
if cls == int:
return "int"
if cls == RawCBOR or cls == RawPlutusData or cls == Datum:
return "any"
if cls == IndefiniteList:
return "list"
if hasattr(cls, "__origin__"):
origin = getattr(cls, "__origin__")
if origin == list:
prefix = "list"
elif origin == dict:
prefix = "map"
elif origin == typing.Union:
prefix = "union"
else:
raise TypeError(
f"Unexpected parameterized type for automatic constructor generation: {cls}"
)
return prefix + "<" + ",".join(id_map(a) for a in cls.__args__) + ">"
if issubclass(cls, PlutusData):
return (
"cons["
+ cls.__name__
+ "]("
+ (str(cls.CONSTR_ID) if not skip_constructor else "_")
+ ";"
+ ",".join(f.name + ":" + id_map(f.type) for f in fields(cls))
+ ")"
)
raise TypeError(f"Unexpected type for automatic constructor generation: {cls}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice. Is there a language-agnostic documentation for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Basically, this is the reference implementation for this kind of constructor IDs. I could write up a documentation, either in this document (where?), OpShin or in a dedicated CIP

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave you to decide. For me, because this can potentially get adopted by other languages, I think a dedicated CIP is the best choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a CIP here: cardano-foundation/CIPs#608

@cffls
Copy link
Collaborator

cffls commented Oct 22, 2023

Tried merging from main branch, but failed with this error:

FAILED test/pycardano/test_plutus.py::test_plutus_data_long_bytes - TypeError: Unexpected type for automatic constructor generation: <class 'pycardano.serialization.Byt...

Looks like ByteString introduced in this PR is not supported by id map yet. Do you think we should add specification for ByteString type, or convert it to list of bytes when generating constructor id?

@nielstron nielstron force-pushed the feat/reconstructible_det_constrids branch from 798c1e3 to 2809b34 Compare October 23, 2023 13:11
@nielstron
Copy link
Contributor Author

I have added support for ByteString now, it should behave the same as bytes (as the existance of ByteString is just an implementation detail of pycardano and irrelevant to third party implementations)

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2023

Codecov Report

Merging #272 (2809b34) into main (2f17b9a) will decrease coverage by 0.02%.
The diff coverage is 83.33%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
- Coverage   84.75%   84.74%   -0.02%     
==========================================
  Files          26       26              
  Lines        3044     3067      +23     
  Branches      740      750      +10     
==========================================
+ Hits         2580     2599      +19     
- Misses        348      350       +2     
- Partials      116      118       +2     
Files Coverage Δ
pycardano/plutus.py 86.69% <83.33%> (-0.37%) ⬇️

Copy link
Collaborator

@cffls cffls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@cffls cffls merged commit af8afdc into Python-Cardano:main Oct 23, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unique constructor id can not be independently implemented in foreign languages
3 participants