-
Notifications
You must be signed in to change notification settings - Fork 0
GP 0.7.1 #370
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request introduces significant refactoring related to the accumulation process, removing the separate on_transfer invocation and integrating its logic into accumulate. It also adds the concept of a registrar privileged service for creating services at specific indices. Several bugs are fixed, and concurrency conformance is improved. However, I found a critical issue in state key generation, a few typos, and have a suggestion for improving debug logs.
Suggestions that couldn't be attached to a specific line
Blockchain/Sources/Blockchain/VMInvocations/HostCall/HostCalls.swift:773, 775, 828, 830, 873, 875, 908-909, 911, 932, 935, 1021, 1023, 1050, 1052, 1102, 1105, 1159, 1161, 1203, 1206, 1256, 1259, 1319, 1321, 1341, 1343
There is a recurring typo in the class name. It should be AccumulateResultContext instead of AccumlateResultContext.
Blockchain/Sources/Blockchain/VMInvocations/Invocations/AccumulateInvocation.swift:56, 61
There is a typo in the class name. It should be AccumulateResultContext instead of AccumlateResultContext.
PolkaVM/Sources/PolkaVM/Engine.swift:104
The instruction name formatting for logging was removed. The previous implementation converted instruction names to uppercase snake_case (e.g., CmovNzImm became CMOV_NZ_IMM), which is arguably more readable in logs. Consider restoring the previous formatting for better debuggability:
return cleanName.replacingOccurrences(of: "([a-z])([A-Z])", with: "$1_$2", options: .regularExpression).uppercased()
PolkaVM/Sources/PolkaVM/Instructions/Instructions.swift:1706
The result of a signed remainder operation with a divisor of zero should be the dividend. The code now correctly writes a (the Int64 dividend) to the destination register. However, the type of the destination register is UInt64. You are writing a signed Int64 to it. It should be context.state.writeRegister(rd, UInt64(bitPattern: a)) to be explicit and safe.
Blockchain/Sources/Blockchain/VMInvocations/InvocationContexts/AccumulateContext.swift
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #370 +/- ##
==========================================
- Coverage 81.93% 81.01% -0.93%
==========================================
Files 381 379 -2
Lines 34012 25899 -8113
==========================================
- Hits 27868 20981 -6887
+ Misses 6144 4918 -1226 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.