Support milliseconds for time delta in Test.moveTime#3015
Support milliseconds for time delta in Test.moveTime#3015m-Peter wants to merge 2 commits intoonflow:masterfrom
Test.moveTime#3015Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| panic(errors.NewUnreachableError()) | ||
| } | ||
| blockchain.MoveTime(int64(timeDelta.ToInt(invocation.LocationRange))) | ||
| blockchain.MoveTime(float64(timeDelta) / sema.Fix64Factor) |
There was a problem hiding this comment.
Casting an int64 to float64 could lose precision in some cases. We could instead pass int64 (Fix64Value as is) to MoveTime, and change the parameter's metric to accept nano-seconds (it currently accepts seconds). But might need to pad-right if we do so, since Fix64Factor is 10^8 and nano-seconds factor 10^9. But then there's a risk of padding-right could overflow/underflow. So IDK which one is better.
In reality, these extreme cases would not be even used, so maybe we don't need to worry much, and just support only up to milliseconds and properly documenting might be enough?
@turbolent wdyt?
There was a problem hiding this comment.
Agreed, we shouldn't use floating point here
3 participants
Description
Although the
timeDeltainTest.moveTimeaccepts aFix64value, under the hood it would truncate the value toint64.We change this to deal with
float64, so that we can support millisecond granularity.Relevant issue: onflow/cadence-tools#264 (comment)
masterbranchFiles changedin the Github PR explorer