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

feat: add support for optional parameters in nano contract serializer/deserializer #647

Merged
merged 2 commits into from
May 10, 2024

Conversation

pedroferreira1
Copy link
Member

Acceptance Criteria

  • Add support in the serializer and deserializer class for optional parameter.
  • Fix bug in the signed data deserializer, where the bytes value was not being converted to hex.

Note: Unfortunately, we currently don't have any blueprints with optional parameters, so we can't test this in the integration tests, thus only unit tests were added.

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@pedroferreira1 pedroferreira1 requested review from tuliomir and r4mmer May 9, 2024 15:53
@pedroferreira1 pedroferreira1 self-assigned this May 9, 2024
Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.63%. Comparing base (cef19d9) to head (fdea37f).

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #647       +/-   ##
===========================================
+ Coverage   62.01%   79.63%   +17.61%     
===========================================
  Files          77       77               
  Lines        5826     5838       +12     
  Branches     1210     1227       +17     
===========================================
+ Hits         3613     4649     +1036     
+ Misses       2116     1172      -944     
+ Partials       97       17       -80     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -22,6 +22,12 @@ class Deserializer {
* @inner
*/
deserializeFromType(value: Buffer, type: string): any {
if (type.endsWith('?')) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a list of types or modifiers?
If so I think we should add a link to it with the see tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another alternative to a @see tag, since we're using Typescript on this repository, would be to add a type for the type parameter.

It's a developer-friendly way to have this information always at hand, and static code validation, on compatible IDEs.

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed in private, we currently don't have this documentation link. Also, the list of possible types is not static, so the documentation link is better.

I created an issue so we can track this when we have the documentation link. #649

@@ -22,6 +22,12 @@ class Deserializer {
* @inner
*/
deserializeFromType(value: Buffer, type: string): any {
if (type.endsWith('?')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another alternative to a @see tag, since we're using Typescript on this repository, would be to add a type for the type parameter.

It's a developer-friendly way to have this information always at hand, and static code validation, on compatible IDEs.

@pedroferreira1 pedroferreira1 force-pushed the feat/support-optional-nano branch from 10c0691 to fdea37f Compare May 10, 2024 16:58
@pedroferreira1 pedroferreira1 merged commit 07e7cbe into master May 10, 2024
4 checks passed
@pedroferreira1 pedroferreira1 deleted the feat/support-optional-nano branch May 10, 2024 18:11
This was referenced May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants