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

Test storing custom types #112

Merged
merged 5 commits into from
Nov 16, 2023

Conversation

julienbrs
Copy link
Contributor

Issue: related to #70

Description

This PR introduces unit tests for storing_custom_type. The main objective of these tests is to demonstrate the storage of custom types within the contract's storage system. Currently, the custom type used does not implement the Serde trait. As a result, we cannot retrieve a return value from the get_person function, which limits its utility. I am considering removing this function due to its limited functionality.

@julio4 julio4 force-pushed the test-storing-custom-types branch from 7d7bc64 to d4fe3f7 Compare November 15, 2023 03:20
@julio4
Copy link
Member

julio4 commented Nov 15, 2023

I've made the following changes:

  • Modified the get_person method to return Person.
  • Altered tests to use ContractState instead of deployment.

The use of contract state is up for discussion. My rationale is that our goal is to demonstrate the syntax, making it unnecessary to directly test deployment.

@julio4 julio4 force-pushed the test-storing-custom-types branch from d4fe3f7 to 402ce26 Compare November 15, 2023 03:31
@julienbrs
Copy link
Contributor Author

julienbrs commented Nov 15, 2023

I've made the following changes:

* Modified the `get_person` method to return `Person`.

* Altered tests to use `ContractState` instead of deployment.

The use of contract state is up for discussion. My rationale is that our goal is to demonstrate the syntax, making it unnecessary to directly test deployment.

Thanks for the review, it indeed makes sense. Following your changes, do we still need the get_person function? It is not used in the test and its usage is already shown in the custom_type_serde example.
I've pushed a version without it, feel free to choose which version to merge.

@julio4 julio4 merged commit ea2ba4e into NethermindEth:main Nov 16, 2023
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.

2 participants