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

fix: optional fields not populated if wire type has additional fields #627

Merged
merged 7 commits into from Sep 20, 2022

Conversation

hpeebles
Copy link
Contributor

@hpeebles hpeebles commented Sep 7, 2022

Description

If the wire type contains more fields than the output type, then (dependent upon the field hash ordering) optional fields in the output type can be skipped even if they match with types on the wire.

How Has This Been Tested?

I have added a unit test which covers this case and all the existing units tests also pass.

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@hpeebles hpeebles requested a review from a team as a code owner September 7, 2022 22:52
@hpeebles
Copy link
Contributor Author

hpeebles commented Sep 7, 2022

This PR makes the assumption that fields are always ordered by their hash values within the type definition. Is that guaranteed by the candid spec?

Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! The field is sorted as guaranteed by the spec.

@nomeata
Copy link
Contributor

nomeata commented Sep 8, 2022

Is this something that the official candid test at https://github.com/dfinity/candid/tree/master/test suite covers?

If yes, it may be worth investing the effort to run agent-js against it somehow?

If not, please extend that test suite maybe?

@chenyan-dfinity
Copy link
Contributor

I will check. The JS implementation isn't doing the subtype checking yet, so we didn't run it through the test suite.

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.

None yet

4 participants