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.

Sign up for GitHub

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

Jump to bottom

[6.0] Global.serialize method #989

Merged
merged 19 commits into from
Sep 20, 2024
Merged

[6.0] Global.serialize method #989

merged 19 commits into from
Sep 20, 2024

Conversation

aslesarenko
Copy link
Member

@aslesarenko aslesarenko commented May 16, 2024 •
edited by kushti
Loading

In this PR:

  • Global.serialize method is added
  • global operation serialize() is added and translated to Global.serializer method call
  • Global.serialize is properly compiled to ErgoTree (tests added)
  • SMethod.explicitTypeArgs added to support serialization of necessary types as part of MethodCall serialization, which cannot be inferred from the MethodCall itself.

@aslesarenko aslesarenko marked this pull request as ready for review May 16, 2024 21:41
@aslesarenko aslesarenko marked this pull request as draft May 17, 2024 07:48
@kushti kushti added this to the v6.0 milestone May 18, 2024
@aslesarenko aslesarenko marked this pull request as ready for review May 19, 2024 15:34
@aslesarenko aslesarenko requested a review from kushti May 19, 2024 15:34
kushti
kushti requested changes May 19, 2024
View reviewed changes
Copy link
Member

@kushti kushti left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are not enough tests:

  • do (de)serialization test for the new methods call in MethodCallSerializerSpecification
  • do serialize - deserialize roundtrip test
  • test newly supported data types , namely Header and Option[], explicitly , but better to test more , see BasicOpsSpecification in deserialize PR

@aslesarenko aslesarenko linked an issue May 21, 2024 that may be closed by this pull request
Global.serialize function to serialize instance of any type #988
Open
@aslesarenko
Copy link
Member Author

  • do serialize - deserialize roundtrip test

  • test newly supported data types , namely Header and Option[], explicitly , but better to test more , see BasicOpsSpecification in deserialize PR

Do you suggest to base this PR on top of #979?

@kushti
Copy link
Member

kushti commented May 22, 2024

  • do serialize - deserialize roundtrip test
  • test newly supported data types , namely Header and Option[], explicitly , but better to test more , see BasicOpsSpecification in deserialize PR

Do you suggest to base this PR on top of #979?

Sure, how else can you check the roundtrip . Also, new serializers (Header & Option) are there

@aslesarenko
Copy link
Member Author

Do you suggest to base this PR on top of #979?

Sure, how else can you check the roundtrip . Also, new serializers (Header & Option) are there

Ok, then this can be postponed until:

@kushti
Copy link
Member

kushti commented May 24, 2024

Do you suggest to base this PR on top of #979?

Sure, how else can you check the roundtrip . Also, new serializers (Header & Option) are there

Ok, then this can be postponed until:

* [[6.0] Merge with refactored SigmaCompiler #987](https://github.com/ScorexFoundation/sigmastate-interpreter/pull/987) is merged

* [[6.0] Global.deserializeTo[] method #979](https://github.com/ScorexFoundation/sigmastate-interpreter/pull/979) is finalized and merged

There is no need to wait for merge of #979. Actually, this PR and #979 will be likely merged about the same time.

@aslesarenko
Copy link
Member Author

aslesarenko commented May 25, 2024 •
edited
Loading

There is no need to wait for merge of #979. Actually, this PR and #979 will be likely merged about the same time.

At least tests should be fixed in #979 before I can merge it into this PR.
Also, #987 should be merged into #979.

@aslesarenko aslesarenko self-assigned this May 25, 2024
Base automatically changed from v6.0.0-refactor-ir-cake to v6.0.0 May 29, 2024 09:57
@kushti kushti mentioned this pull request Jun 4, 2024
Closed
@kushti kushti assigned kushti and unassigned aslesarenko Jun 6, 2024
@kushti kushti changed the base branch from v6.0.0 to v6.0-newfeature June 6, 2024 09:38
@kushti kushti mentioned this pull request Jun 7, 2024
Merged
@kushti
Copy link
Member

kushti commented Jun 7, 2024

TODO:

@aslesarenko
Copy link
Member Author

aslesarenko commented Jun 8, 2024 •
edited
Loading

  • check if a specific serializer w. costing possible / makes sense

Please clarify:

  • Which specific serializer?
  • what "possible / makes sense" means?

Also, please fix the tests.

@kushti
Copy link
Member

kushti commented Jun 8, 2024 •
edited
Loading

@ergomorphic those are notes for myself, the PR is not finished, please review finished PRs! In particular, to fix the tests, #972 with Header serialization should be merged into v6.0.0 branch and then this PR.

@kushti kushti force-pushed the v6.0-newfeature branch from 07b8644 to e7bd2a3 Compare June 8, 2024 18:49
Base automatically changed from v6.0-newfeature to v6.0.0 June 9, 2024 08:53
@kushti kushti mentioned this pull request Jun 9, 2024
Open
@kushti kushti mentioned this pull request Aug 14, 2024
Closed
@SethDusek SethDusek mentioned this pull request Sep 17, 2024
Open
@aslesarenko
Copy link
Member Author

@kushti, LGTM, cannot approve my own PR, consider approved.

@kushti kushti merged commit f5feee5 into v6.0.0 Sep 20, 2024
4 checks passed
This was referenced Sep 20, 2024
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kushti kushti kushti approved these changes

Assignees

@kushti kushti

Labels
None yet
Projects
None yet
Milestone
v6.0
Development

Successfully merging this pull request may close these issues.

Global.serialize function to serialize instance of any type
2 participants
@aslesarenko @kushti