Implemented string_to_account() function expression #28
Conversation
WalkthroughThe changes introduce new functionalities for handling account-related operations in the interpreter module. A constant for converting strings to accounts is added, along with new functions for managing metadata and evaluating expressions involving strings. The ability to concatenate strings and convert them to account representations is enhanced, while several older functions related to balance and metadata retrieval are removed. Additionally, corresponding test cases are introduced to validate these new features and ensure proper functionality. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 63.24% 63.29% +0.05%
==========================================
Files 29 31 +2
Lines 6456 6482 +26
==========================================
+ Hits 4083 4103 +20
- Misses 2191 2195 +4
- Partials 182 184 +2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
internal/interpreter/function_expressions.go (4)
18-18: Reminder: Implement precise error location handlingThe TODO comment on line 18 (
// TODO more precise location) indicates that the error ranges in thestringToAccountfunction could be made more precise. Enhancing error location precision can significantly improve debugging and user feedback.
34-34: Reminder: Implement precise error location handlingThe TODO comment on line 34 suggests improving the precision of error ranges in the
metafunction. Providing exact locations for errors can aid in quicker diagnosis and resolution.
67-67: Reminder: Implement precise error location handlingIn the
balancefunction, the TODO comment on line 67 (// TODO more precise args range location) highlights the need for precise argument range handling. Consider refining this to enhance error reporting.
108-108: Reminder: Implement precise error location handlingThe TODO on line 108 in the
overdraftfunction indicates that argument range precision needs attention. Addressing this will improve the clarity of error messages.internal/interpreter/infix.go (1)
28-35: Handle non-string operands in string concatenationThe
evalAddmethod for theStringtype enables string concatenation. While the current implementation assumes that the right operand is a string, consider adding explicit error messages or type checks for cases where the operand is not a string to improve robustness.internal/analysis/check.go (1)
85-89: Consider enhancing the function documentation.While the current documentation is clear, it would be helpful to add more details about:
- Expected string format for valid account names
- Behavior with empty strings or invalid characters
- Any length restrictions
FnVarOriginStringToAccount: VarOriginFnCallResolution{ Params: []string{TypeString}, Return: TypeAccount, - Docs: "cast a string to an account", + Docs: "cast a string to an account. The string must be a valid account name following numscript's grammar rules. Empty strings and invalid characters will result in an error.", },internal/interpreter/interpreter_test.go (1)
3547-3562: Add more test cases for string concatenation.While the basic test is good, consider adding test cases for:
- Empty strings:
"" + "abc","abc" + "","" + ""- Long strings to verify any length limitations
- Special characters and Unicode strings
- Multiple concatenations:
"a" + "b" + "c"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
internal/analysis/check.go(2 hunks)internal/interpreter/evaluate_expr.go(1 hunks)internal/interpreter/function_expressions.go(1 hunks)internal/interpreter/function_statements.go(1 hunks)internal/interpreter/infix.go(2 hunks)internal/interpreter/interpreter.go(4 hunks)internal/interpreter/interpreter_test.go(2 hunks)
🔇 Additional comments (8)
internal/interpreter/function_expressions.go (1)
143-151: Verify error handling in getBalance function
Ensure that the error wrapping and handling in the getBalance function provide sufficient context. When s.runBalancesQuery() returns an error, confirming that QueryBalanceError includes all necessary information will aid in debugging.
internal/interpreter/interpreter.go (4)
150-152: Integration of stringToAccount function in handleOrigin
The addition of the case for analysis.FnVarOriginStringToAccount correctly integrates the new stringToAccount function into the handleOrigin method. This allows the interpreter to handle string-to-account conversions as intended.
185-188: Consistency in feature flag declarations
The feature flag ExperimentalStringToAccountFunctionFeatureFlag is consistently declared alongside ExperimentalOverdraftFunctionFeatureFlag. This naming convention maintains clarity and aligns with existing feature flags.
213-216: Feature flag activation for StringToAccountFunction
The code correctly checks for the ExperimentalStringToAccountFunctionFeatureFlag and activates the corresponding feature in the RunProgram function. This ensures the experimental feature is only enabled when intended.
274-275: Addition of StringToAccountFunctionFeatureFlag to programState
The new boolean field StringToAccountFunctionFeatureFlag is added to the programState struct. Verify that this addition does not introduce unintended side effects, especially regarding struct initialization and memory alignment.
internal/interpreter/function_statements.go (1)
32-32: Confirm correct string conversion of metadata values
On line 32, the meta value is converted to a string using (*meta).String(). Ensure that all meta values can be accurately represented as strings and that this conversion does not lose important information, especially if meta values might be of non-string types.
internal/interpreter/evaluate_expr.go (1)
103-105: Extension of plusOp to support string concatenation
By mapping expectString in the plusOp function, the code now supports string concatenation using the "+" operator. This extension aligns with the overloaded operator functionality. Ensure that this addition does not conflict with other type operations and that adequate test coverage is in place.
internal/analysis/check.go (1)
59-59: LGTM! The constant declaration follows the established pattern.
The constant name and value are consistent with other function declarations in the codebase.
| return err | ||
| } | ||
|
|
||
| st.TxMeta[*key] = *meta |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure thread safety when modifying TxMeta
In the setTxMeta function, assigning to st.TxMeta[*key] occurs without synchronization. If programState may be accessed concurrently, consider implementing mutex locks or other synchronization mechanisms to prevent potential data races.
| func TestToAccount(t *testing.T) { | ||
| script := ` | ||
| vars { | ||
| account $acc = string_to_account("account_name") | ||
| } | ||
|
|
||
| send [COIN 10] ( | ||
| source = @world | ||
| destination = $acc | ||
| ) | ||
| ` | ||
|
|
||
| tc := NewTestCase() | ||
| tc.compile(t, script) | ||
|
|
||
| tc.expected = CaseResult{ | ||
| Postings: []Posting{ | ||
| { | ||
| Source: "world", | ||
| Destination: "account_name", | ||
| Amount: big.NewInt(10), | ||
| Asset: "COIN", | ||
| }, | ||
| }, | ||
| } | ||
| testWithFeatureFlag(t, tc, machine.ExperimentalStringToAccountFunctionFeatureFlag) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for string_to_account function.
The current test only covers the happy path. Consider adding test cases for:
- Invalid account names
- Empty strings
- Special characters
- Very long strings
- Account names with spaces or invalid characters
Also, consider extracting the feature flag constant to avoid magic strings:
+const experimentalStringToAccountTestFlag = machine.ExperimentalStringToAccountFunctionFeatureFlag
func TestToAccount(t *testing.T) {
// ... test code ...
- testWithFeatureFlag(t, tc, machine.ExperimentalStringToAccountFunctionFeatureFlag)
+ testWithFeatureFlag(t, tc, experimentalStringToAccountTestFlag)
}
+func TestToAccountErrors(t *testing.T) {
+ testCases := []struct {
+ name string
+ accountName string
+ expectedError machine.InterpreterError
+ }{
+ {"empty_string", "", machine.InvalidAccountName{Name: ""}},
+ {"invalid_chars", "account@name", machine.InvalidAccountName{Name: "account@name"}},
+ // Add more test cases
+ }
+ // ... implement test cases
+}Committable suggestion skipped: line range outside the PR's diff.
This function implements two distinct functionalities:
string_to_accountfunction which can be used to cast strings into accountFeature flags
feature 2) is under the
--experimental-string-to-account-functionfeature flag.The string concatenation feature IS NOT under a feature flag. This means merging this PR will be equivalent as considering this functionality stable (in the numscript rewrite). This is discussed in the next paragraph
Design consideration for string concatenation
It's safe to assume current design of string concatenation is good enough to be merged, e.g. we won't need any breaking changes.
While operator overloading's cost is to prevent a fully fledged type inference,
Other further development of the functionality will need more considerations, e.g. either forbidding
"ab" + 2(current behaviour) or casting2to a string. The latter is, however, not a breaking changeOpen ended questions