-
Notifications
You must be signed in to change notification settings - Fork 4
feat: asset scaling #102
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
base: main
Are you sure you want to change the base?
feat: asset scaling #102
Changes from all commits
0ed13f4
ad85f52
5d92251
2e7ee44
7bcf3cc
2ec6835
437d975
8c06296
75f9409
1d4302f
5e73a3b
f1e631e
2105765
bf5d543
6fe39e8
85d6db4
c250b5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -523,3 +523,32 @@ send [ASSET *] ( | |||||||||||||||||||||||||||||||||||
| resolved := checkResult.ResolveVar(variableHover.Node) | ||||||||||||||||||||||||||||||||||||
| require.NotNil(t, resolved) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| func TestHover(t *testing.T) { | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| input := `vars { | ||||||||||||||||||||||||||||||||||||
| account $x | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| send [EUR/2 *] ( | ||||||||||||||||||||||||||||||||||||
| source = { | ||||||||||||||||||||||||||||||||||||
| @acc1 | ||||||||||||||||||||||||||||||||||||
| @acc2 with scaling through $x | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| destination = @dest | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| ` | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| rng := parser.RangeOfIndexed(input, "$x", 1) | ||||||||||||||||||||||||||||||||||||
| program := parser.Parse(input).Value | ||||||||||||||||||||||||||||||||||||
| hover := analysis.HoverOn(program, rng.Start) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| varHover, ok := hover.(*analysis.VariableHover) | ||||||||||||||||||||||||||||||||||||
| if !ok { | ||||||||||||||||||||||||||||||||||||
| t.Fatalf("Expected a VariableHover") | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| require.Equal(t, varHover.Node.Name, "x") | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+545
to
+552
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing nil check and incorrect
Proposed fix rng := parser.RangeOfIndexed(input, "$x", 1)
program := parser.Parse(input).Value
hover := analysis.HoverOn(program, rng.Start)
+ require.NotNil(t, hover)
varHover, ok := hover.(*analysis.VariableHover)
if !ok {
t.Fatalf("Expected a VariableHover")
}
- require.Equal(t, varHover.Node.Name, "x")
+ require.Equal(t, "x", varHover.Node.Name)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| package interpreter | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "math/big" | ||
| "slices" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/formancehq/numscript/internal/utils" | ||
| ) | ||
|
|
||
| func assetToScaledAsset(asset string) string { | ||
| parts := strings.Split(asset, "/") | ||
| if len(parts) == 1 { | ||
| return asset + "/*" | ||
| } | ||
| return parts[0] + "/*" | ||
| } | ||
|
|
||
| func buildScaledAsset(baseAsset string, scale int64) string { | ||
| if scale == 0 { | ||
| return baseAsset | ||
| } | ||
| return fmt.Sprintf("%s/%d", baseAsset, scale) | ||
| } | ||
|
|
||
| func getAssetScale(asset string) (string, int64) { | ||
| parts := strings.Split(asset, "/") | ||
| if len(parts) == 2 { | ||
| scale, err := strconv.ParseInt(parts[1], 10, 64) | ||
| if err == nil { | ||
| return parts[0], scale | ||
| } | ||
| // fallback if parsing fails | ||
| return parts[0], 0 | ||
| } | ||
| return asset, 0 | ||
| } | ||
|
|
||
| func getAssets(balance AccountBalance, baseAsset string) map[int64]*big.Int { | ||
| result := make(map[int64]*big.Int) | ||
| for asset, amount := range balance { | ||
| if strings.HasPrefix(asset, baseAsset) { | ||
| _, scale := getAssetScale(asset) | ||
| result[scale] = amount | ||
| } | ||
| } | ||
| return result | ||
| } | ||
|
Comment on lines
+41
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefix matching may cause false positives.
🔎 Proposed fix func getAssets(balance AccountBalance, baseAsset string) map[int64]*big.Int {
result := make(map[int64]*big.Int)
for asset, amount := range balance {
- if strings.HasPrefix(asset, baseAsset) {
+ base, _ := getAssetScale(asset)
+ if base == baseAsset {
_, scale := getAssetScale(asset)
result[scale] = amount
}
}
return result
}🤖 Prompt for AI Agents |
||
|
|
||
| type scalePair struct { | ||
| scale int64 | ||
| amount *big.Int | ||
| } | ||
|
|
||
| // e.g. | ||
| // | ||
| // need=[EUR/2 100], got={EUR/2: 100, EUR: 1} | ||
| // => {EUR/2: 100, EUR: 1} | ||
| // | ||
| // need=[EUR 1], got={EUR/2: 100, EUR: 0} | ||
| // => {EUR/2: 100, EUR: 0} | ||
| // | ||
| // need=[EUR/2 199], got={EUR/2: 100, EUR: 2} | ||
| // => {EUR/2: 100, EUR: 1} | ||
| func findSolution( | ||
| neededAmt *big.Int, // <- can be nil | ||
| neededAmtScale int64, | ||
| scales map[int64]*big.Int, | ||
| ) ([]scalePair, *big.Int) { | ||
| var assets []scalePair | ||
| for k, v := range scales { | ||
| assets = append(assets, scalePair{ | ||
| scale: k, | ||
| amount: v, | ||
| }) | ||
| } | ||
|
|
||
| // Sort in ASC order (e.g. EUR, EUR/2, ..) | ||
| slices.SortFunc(assets, func(p scalePair, other scalePair) int { | ||
| return int(p.scale - other.scale) | ||
| }) | ||
|
|
||
| var out []scalePair | ||
|
|
||
| totalSent := big.NewInt(0) | ||
|
|
||
| for _, p := range assets { | ||
| scaleDiff := neededAmtScale - p.scale | ||
|
|
||
| exp := big.NewInt(scaleDiff) | ||
| exp.Abs(exp) | ||
| exp.Exp(big.NewInt(10), exp, nil) | ||
|
|
||
| // scalingFactor := 10 ^ (neededAmtScale - p.scale) | ||
| // note that 10^0 == 1 and 10^(-n) == 1/(10^n) | ||
| scalingFactor := new(big.Rat).SetInt(exp) | ||
| if scaleDiff < 0 { | ||
| scalingFactor.Inv(scalingFactor) | ||
| } | ||
|
|
||
| allowed := new(big.Int).Mul(p.amount, scalingFactor.Num()) | ||
| allowed.Div(allowed, scalingFactor.Denom()) | ||
|
|
||
| var leftAmt *big.Int | ||
| var taken *big.Int | ||
| if neededAmt == nil { | ||
| taken = new(big.Int).Set(allowed) | ||
| } else { | ||
| leftAmt = new(big.Int).Sub(neededAmt, totalSent) | ||
| taken = utils.MinBigInt(allowed, leftAmt) | ||
| } | ||
|
|
||
| totalSent.Add(totalSent, taken) | ||
|
|
||
| intPart := new(big.Int).Mul(taken, scalingFactor.Denom()) | ||
| intPart.Div(intPart, scalingFactor.Num()) | ||
|
|
||
| if intPart.Cmp(big.NewInt(0)) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| out = append(out, scalePair{ | ||
| scale: p.scale, | ||
| amount: intPart, | ||
| }) | ||
|
|
||
| if leftAmt != nil && leftAmt.Cmp(big.NewInt(0)) != 1 { | ||
| break | ||
| } | ||
|
Comment on lines
+129
to
+131
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Break condition may not trigger correctly.
🔎 Proposed fixRecompute the remaining amount after updating totalSent.Add(totalSent, taken)
intPart := new(big.Int).Mul(taken, scalingFactor.Denom())
intPart.Div(intPart, scalingFactor.Num())
if intPart.Cmp(big.NewInt(0)) == 0 {
continue
}
out = append(out, scalePair{
scale: p.scale,
amount: intPart,
})
- if leftAmt != nil && leftAmt.Cmp(big.NewInt(0)) != 1 {
+ if neededAmt != nil {
+ remaining := new(big.Int).Sub(neededAmt, totalSent)
+ if remaining.Cmp(big.NewInt(0)) <= 0 {
+ break
+ }
+ }
- break
- }
}
🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai give me tests that can reproduce the error. Or describe them if you can't reproduce the numscript exactly
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ascandone, here's a test case that demonstrates the issue: func TestScalingBreakConditionStale(t *testing.T) {
// Setup: Need exactly 100 at scale 0
// Available: scale 0 has 100, scale 1 has 1000
// Expected: Should only use scale 0 (since it satisfies the need exactly)
// Bug: The function continues to the next iteration unnecessarily
neededAmt := big.NewInt(100)
neededScale := int64(0)
scales := map[int64]*big.Int{
0: big.NewInt(100), // Exactly what we need
1: big.NewInt(1000), // Extra available at higher scale
}
result, totalSent := findSolution(neededAmt, neededScale, scales)
// After processing scale 0:
// - leftAmt = 100 - 0 = 100 (before update)
// - taken = 100
// - totalSent = 100
// - Check: leftAmt (100) <= 0? False → continues to next iteration
//
// Next iteration (scale 1):
// - leftAmt = 100 - 100 = 0
// - taken = 0
// - Gets skipped due to intPart == 0 check
//
// While functionally correct, the break condition should have
// terminated after the first iteration since remaining = 0
assert.Equal(t, big.NewInt(100), totalSent)
assert.Equal(t, 1, len(result)) // Should only have scale 0
assert.Equal(t, int64(0), result[0].scale)
assert.Equal(t, big.NewInt(100), result[0].amount)
}Description of the issue: The problem occurs when an asset scale exactly satisfies the remaining need. In the example above:
While this doesn't produce incorrect results (due to the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai I ran this test as-is and it's passing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ascandone, you're absolutely right — the test passes because the code is functionally correct. I need to clarify my original assessment: What actually happens:
The verdict: This is a minor inefficiency (one unnecessary loop iteration), not a correctness bug. The The suggested fix would make the code slightly cleaner and more efficient by breaking immediately when the need is satisfied, but it's not critical. If you prefer to keep the code as-is for simplicity, that's reasonable. Thank you for verifying — this helped clarify the actual impact! 🙏 |
||
| } | ||
|
|
||
| return out, totalSent | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| package interpreter | ||
|
|
||
| import ( | ||
| "math/big" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestScalingZeroNeeded(t *testing.T) { | ||
| t.Skip() | ||
|
|
||
| // need [EUR/2 ] | ||
| sol, _ := findSolution( | ||
| big.NewInt(0), | ||
| 42, | ||
| map[int64]*big.Int{ | ||
| 2: big.NewInt(100), | ||
| 1: big.NewInt(1), | ||
| }) | ||
|
|
||
| require.Equal(t, []scalePair{ | ||
| {42, big.NewInt(0)}, | ||
| }, sol) | ||
| } | ||
|
|
||
| func TestScalingSameAsset(t *testing.T) { | ||
| sol, _ := findSolution( | ||
| // Need [EUR/2 200] | ||
| big.NewInt(200), | ||
| 2, | ||
|
|
||
| // Have: {EUR/2: 201} | ||
| map[int64]*big.Int{ | ||
| 2: big.NewInt(201), | ||
| }) | ||
|
|
||
| require.Equal(t, []scalePair{ | ||
| {2, big.NewInt(200)}, | ||
| }, sol) | ||
| } | ||
|
|
||
| func TestScalingSolutionLowerScale(t *testing.T) { | ||
| sol, _ := findSolution( | ||
| big.NewInt(1), | ||
| 0, | ||
| map[int64]*big.Int{ | ||
| 2: big.NewInt(900), | ||
| }) | ||
|
|
||
| require.Equal(t, []scalePair{ | ||
| {2, big.NewInt(100)}, | ||
| }, sol) | ||
| } | ||
|
|
||
| func TestScalingSolutionHigherScale(t *testing.T) { | ||
| sol, _ := findSolution( | ||
| // Need [EUR/2 200] | ||
| big.NewInt(200), | ||
| 2, | ||
|
|
||
| // Have: {EUR: 4} (eq to EUR/2 400) | ||
| map[int64]*big.Int{ | ||
| 0: big.NewInt(4), | ||
| }) | ||
|
|
||
| require.Equal(t, []scalePair{ | ||
| {0, big.NewInt(2)}, | ||
| }, sol) | ||
| } | ||
|
|
||
| func TestScalingSolutionHigherScaleNoSolution(t *testing.T) { | ||
| sol, _ := findSolution( | ||
| big.NewInt(1), | ||
| 2, | ||
| map[int64]*big.Int{ | ||
| 0: big.NewInt(100), | ||
| 1: big.NewInt(100), | ||
| }) | ||
|
|
||
| require.Nil(t, sol) | ||
| } | ||
|
|
||
| func TestMixedFail(t *testing.T) { | ||
| t.Skip() | ||
|
|
||
| sol, _ := findSolution( | ||
| big.NewInt(400), | ||
| 2, | ||
| map[int64]*big.Int{ | ||
| 0: big.NewInt(1), | ||
| 2: big.NewInt(200), | ||
| 3: big.NewInt(10), | ||
| }) | ||
|
|
||
| require.Nil(t, sol) | ||
| } | ||
|
|
||
| func TestUnboundedScalingSameAsset(t *testing.T) { | ||
| sol, _ := findSolution( | ||
| nil, | ||
| // Need USD/2 | ||
| 2, | ||
| // Have: {EUR/2: 201} | ||
| map[int64]*big.Int{ | ||
| 2: big.NewInt(123), | ||
| }) | ||
|
|
||
| require.Equal(t, []scalePair{ | ||
| {2, big.NewInt(123)}, | ||
| }, sol) | ||
| } | ||
|
|
||
| func TestUnboundedScalingLowerAsset(t *testing.T) { | ||
| sol, _ := findSolution( | ||
| nil, | ||
| 2, | ||
| map[int64]*big.Int{ | ||
| 0: big.NewInt(1), | ||
| }) | ||
|
|
||
| require.Equal(t, []scalePair{ | ||
| {0, big.NewInt(1)}, | ||
| }, sol) | ||
| } | ||
|
|
||
| func TestUnboundedScalinHigherAsset(t *testing.T) { | ||
| sol, _ := findSolution( | ||
| nil, | ||
| 2, | ||
| map[int64]*big.Int{ | ||
| 3: big.NewInt(10), | ||
| }) | ||
|
|
||
| require.Equal(t, | ||
| []scalePair{ | ||
| {3, big.NewInt(10)}, | ||
| }, | ||
| sol) | ||
| } | ||
|
|
||
| func TestUnboundedScalinHigherAssetTrimRemainder(t *testing.T) { | ||
| sol, _ := findSolution( | ||
| nil, | ||
| 2, | ||
| map[int64]*big.Int{ | ||
| 3: big.NewInt(15), | ||
| }) | ||
|
|
||
| require.Equal(t, []scalePair{ | ||
| {3, big.NewInt(10)}, | ||
| }, sol) | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.