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

Add fromBigEndianBytes conversion function to Number types #2496

Merged
merged 10 commits into from
May 24, 2023

Conversation

darkdrag00nv2
Copy link
Contributor

@darkdrag00nv2 darkdrag00nv2 commented May 12, 2023

Closes #2448
Work towards onflow/developer-grants#179

Description

Added fromBigEndianBytes to all the Number types - Int*, UInt*, Fix*, Word*.

We return Nil in case of invalid input such as overflow. Note that we also return Nil if the size of the byte array is greater than the size needed for the target type. E.g. [0, 0] does not lead to overflow for UInt8 but we return Nil since we expect the byte array size to only be 1.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #2496 (57f4b9c) into master (043b805) will increase coverage by 0.04%.
The diff coverage is 92.52%.

@@            Coverage Diff             @@
##           master    #2496      +/-   ##
==========================================
+ Coverage   78.35%   78.39%   +0.04%     
==========================================
  Files         327      327              
  Lines       73044    73262     +218     
==========================================
+ Hits        57235    57436     +201     
- Misses      13710    13720      +10     
- Partials     2099     2106       +7     
Flag Coverage Δ
unittests 78.39% <92.52%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/interpreter/interpreter.go 89.89% <90.24%> (+0.01%) ⬆️
runtime/interpreter/big.go 89.47% <100.00%> (+5.68%) ⬆️
runtime/sema/type.go 89.52% <100.00%> (+0.07%) ⬆️

... and 1 file with indirect coverage changes

@turbolent
Copy link
Member

Just realized that #1917 fixed the behaviour for toBigEndianBytes on the Stable Cadence feature branch (feature/stable-cadence).
Maybe we should target this PR against Stable Cadence too @onflow/cadence ?

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved
runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved
@darkdrag00nv2
Copy link
Contributor Author

Maybe we should target this PR against Stable Cadence too @onflow/cadence ?

@turbolent sure, I can switch the branch to stable cadence if needed. In future, when the stable cadence branch is fully merged into master, these changes will also come in. Do let me know.

@SupunS
Copy link
Member

SupunS commented May 19, 2023

Just realized that #1917 fixed the behaviour for toBigEndianBytes on the Stable Cadence feature branch (feature/stable-cadence).
Maybe we should target this PR against Stable Cadence too ?

Sorry, I missed this. Does the new API fromBigEndianBytes work with fixed-size inputs or variable-sized inputs (both)?
If it seamlessly works for both, then I see no harm in merging this to master.

Otherwise, If we'll have to do any adjustments, then merging to Stable Cadence directly sounds like a good idea. It would save us the trouble of having to maintain two versions of this / update to match the Stable cadence later on (which could fall through the cracks). But like I said, if this is not the case, then I'm OK with merging to master.

@darkdrag00nv2
Copy link
Contributor Author

Does the new API fromBigEndianBytes work with fixed-size inputs or variable-sized inputs (both)?
If it seamlessly works for both, then I see no harm in merging this to master.

@SupunS Yes, it works for both the cases. For variable-sized inputs, we add the necessary padding.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice work!

@turbolent
Copy link
Member

@onflow/cadence could you please have a look?

@turbolent turbolent merged commit 052f79f into onflow:master May 24, 2023
9 of 14 checks passed
@darkdrag00nv2 darkdrag00nv2 deleted the from_bigendianbytes_2448 branch May 25, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add fromBigEndianBytes conversion function to Number types
4 participants