Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Oct 14, 2025

Closes #4127

Description

This PR upgrades Fix64Value and UFix64Value to use the github.com/onflow/fixed-point library, following the same pattern as Fix128Value/UFix128Value. This is a draft PR for initial review.

Link to Devin run: https://app.devin.ai/sessions/3d6a099b4b7b4a5c84c4162298b7922f
Requested by: [email protected]

Key Changes

  1. Type definitions (interpreter/value_fix64.go, interpreter/value_ufix64.go):

    • Changed from type Fix64Value int64 to type Fix64Value fix.Fix64
    • Changed from type UFix64Value struct { UFix64Value values.UFix64Value } to type UFix64Value fix.UFix64
    • Updated all constructors to use func() fix.Fix64 and func() fix.UFix64 callbacks
    • Arithmetic operations now call library methods: .Add(), .Sub(), .Mul(), .Div(), .Mod()
    • Comparison operations now use: .Lt(), .Lte(), .Gt(), .Gte()
  2. Added utility functions (fixedpoint/convert.go, fixedpoint/check.go):

    • Fix64FromBigInt(), Fix64ToBigInt(), UFix64FromBigInt(), UFix64ToBigInt()
    • Fix64FactorAsBigInt and UFix64FactorAsBigInt constants
  3. Updated call sites:

    • bbq/compiler/compiler.go: Cast values when creating constants
    • interpreter/decode.go: Updated CBOR decoding
    • interpreter/interpreter.go: Fixed string parsers and min/max constants
    • stdlib/account.go: Added fix.UFix64(balance) conversions for account balances

Critical Areas for Review

⚠️ This is a draft PR with limited testing - per the original request, extensive testing was deferred.

  1. Type conversions in stdlib/account.go:484,510: Verify that fix.UFix64(balance) correctly converts raw uint64 balance values to the scaled fixed-point representation

  2. Saturation arithmetic behavior: The old code used custom SaturatingPlus/SaturatingMinus methods; the new code manually handles overflow by returning fix.UFix64Max/fix.UFix64Zero. Verify semantics match.

  3. Storage encoding/decoding: Changes to CBOR encoding in value_ufix64.go - verify roundtrip correctness

  4. Arithmetic precision: All arithmetic operations now use fix.RoundTruncate - verify this matches expected behavior

  5. Interface completeness: While the code compiles and implements atree.Storable, NumberValue, etc., verify all requirements are met

Testing Status

  • ✅ Local build successful
  • ⚠️ CI failures fixed (balance type conversions)
  • ❌ Full test suite not run yet
  • ❌ No new tests added

Checklist:

  • Targeted PR against master branch
  • Linked to Github issue Upgrade [U]Fix64 to new implementation #4127
  • Code follows the standards mentioned in CONTRIBUTING.md
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels
  • Run full test suite and verify all tests pass
  • Verify saturation arithmetic semantics match old implementation
  • Verify storage encoding/decoding roundtrips correctly

…ixedpoint helpers and update call sites; draft for review
@devin-ai-integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@github-actions
Copy link

Benchstat comparison

  • Base branch: onflow:master
  • Base commit: 3ec0c24
Results

old.txtnew.txt
time/opdelta
pkg:github.com/onflow/cadence/sema goos:linux goarch:amd64
QualifiedIdentifierCreation/One_level-42.49ns ± 0%2.50ns ± 0%~(p=1.000 n=1+1)
QualifiedIdentifierCreation/Three_levels-485.5ns ± 0%85.4ns ± 0%~(p=1.000 n=1+1)
SuperTypeInference/arrays-4234ns ± 0%241ns ± 0%~(p=1.000 n=1+1)
SuperTypeInference/composites-490.1ns ± 0%93.0ns ± 0%~(p=1.000 n=1+1)
SuperTypeInference/integers-4307ns ± 0%310ns ± 0%~(p=1.000 n=1+1)
 
alloc/opdelta
pkg:github.com/onflow/cadence/sema goos:linux goarch:amd64
QualifiedIdentifierCreation/One_level-40.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-464.0B ± 0%64.0B ± 0%~(all equal)
SuperTypeInference/arrays-496.0B ± 0%96.0B ± 0%~(all equal)
SuperTypeInference/composites-40.00B 0.00B ~(all equal)
SuperTypeInference/integers-40.00B 0.00B ~(all equal)
 
allocs/opdelta
pkg:github.com/onflow/cadence/sema goos:linux goarch:amd64
QualifiedIdentifierCreation/One_level-40.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-42.00 ± 0%2.00 ± 0%~(all equal)
SuperTypeInference/arrays-43.00 ± 0%3.00 ± 0%~(all equal)
SuperTypeInference/composites-40.00 0.00 ~(all equal)
SuperTypeInference/integers-40.00 0.00 ~(all equal)
 

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.

Upgrade [U]Fix64 to new implementation

1 participant