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

Allow enum name payload #177

Merged
merged 6 commits into from
Jan 17, 2025

Conversation

EtienneWallet
Copy link
Contributor

Context

When supplying the payload of a an enum which might contains fields, the user has to provide the discriminant.
This doesn't seem very user friendly as in a smart-contract the user defines the variants by names and the names hold much more meaning.

Proposed changes

I propose to allow the direct use of variant names when setting the payload of an EnumValue.
The logic is to detect when a name was provided instead of a discriminant and convert it before resuming operations.
For that, a dictionary names_to_discriminants was introduced to allow the user to provide the conversion means from variant names to variant discriminants

For example, if the user provides a list payload, this would allow to go from [44, 7, 8] to ["VariantD", 7, 8]

Copy link

github-actions bot commented Jan 15, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  multiversx_sdk/abi
  abi.py
  constants.py
  enum_value.py 85, 110, 120
Project Total  

This report was generated by python-coverage-comment-action

Comment on lines 69 to 70
if self.names_to_discriminants is None:
raise ValueError("populating an enum from a native object requires the names to discriminants dict to be set")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe skip this check? I think it will help allow for the old flow of initialization.
e.g. someone manually initializes an EnumValue(fields_provider=...) and then calls set_payload()

Copy link
Contributor Author

@EtienneWallet EtienneWallet Jan 16, 2025

Choose a reason for hiding this comment

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

The issue I have with removing this check is that we would have to create three duplicates checks each time we want to use names_to_discriminants within the function set_payload.
I would argue that this brings unnecessary verbosity and is not coherent with how fields_provider is treated

A third way maybe: I haven't consider how all the classes are configured but could we create a class method constructor EnumValue.from_definition(cls, def : EnumDefinition) -> EnumValue ? This would give the user a simple way to create the EnumValue without having to worry about fields_provider or names_to_discriminants.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We want to allow the old flow, so I think removing the check here is still the way to go. Regarding duplicating the code, that can be a extracted in a private method that will be called wherever we use self.names_to_discriminants. Maybe something like:

def _self.ensure_names_to_discriminants_is_set(self):
    if self.names_to_discriminants is None:
            raise ValueError("...")

The idea about EnumValue.from_definition(...) is great and we should do that for StructValue, as well. Let's do it in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The switch to lazy check has been made as you wanted
instead of having a function that only does the check, I preferred to create a public function to convert a variant name to its discriminant. This way, we are sure that the check is done and the conversion is included
If we ever need to use names_to_discriminants differently, we could always separate the check and the conversion

Don't hesitate to tell me if this doesn't suit you, I'll change it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to me!

@popenta popenta merged commit 8280297 into multiversx:feat/next Jan 17, 2025
4 checks passed
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.

3 participants