Conversation
This reverts commit b8909f4.
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
Addressed review item #117 in commit f19b80b. The auth endpoint still returns the same plain auth_ok response shape, but the dead private-key load/signing path and unsafe cast in src/libs/network/manageAuth.ts were removed because the signature was never returned or consumed by the current peer handshake flow. |
|
This PR modifies Files that will be reverted:
|
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
![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
♻️ Duplicate comments (2)
src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts (2)
1351-1371:⚠️ Potential issue | 🔴 CriticalRollback must restore the previous script payload.
This is a previously flagged critical issue that remains unaddressed. When
previousVersionis provided, line 1355 only restoresscriptVersionbut leavestoken.scriptcontaining the NEW upgraded code. The token will claim an older version while executing the upgraded script.To properly rollback, the edit must carry the previous script code (e.g.,
edit.data.previousScript) and restore it:if (edit.isRollback) { if (previousVersion !== undefined && previousVersion >= 0) { token.scriptVersion = previousVersion + // Restore actual previous script payload + if (edit.data.previousScript !== undefined) { + token.script = edit.data.previousScript + token.hasScript = !!edit.data.previousScript?.code + token.lastScriptUpdate = edit.data.previousLastScriptUpdate ?? null + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts` around lines 1351 - 1371, In the rollback branch (when edit.isRollback) you're only restoring token.scriptVersion but not the prior script payload; update the logic in the block that checks previousVersion to also read the prior script from the edit payload (e.g., edit.data.previousScript) and set token.script = edit.data.previousScript, token.hasScript = Boolean(edit.data.previousScript), token.scriptVersion = previousVersion, and update token.lastScriptUpdate appropriately (or preserve a provided previous timestamp if available like edit.data.previousLastUpdate); keep the existing else branch that clears the script when no previousVersion/info is provided.
1458-1465:⚠️ Potential issue | 🔴 CriticalRollback needs the recorded previous owner.
This is a previously flagged critical issue that remains unaddressed. Line 1462 assumes
edit.account(the caller) was the previous owner, but ACL delegates can transfer ownership on behalf of others.The edit must record the actual previous owner (e.g.,
edit.data.previousOwner) when the transfer is created:// For rollback, swap back if (edit.isRollback) { - token.owner = edit.account // Previous owner was the caller + if (edit.data.previousOwner) { + token.owner = edit.data.previousOwner + } else { + log.warn("[GCRTokenRoutines] Ownership rollback without previousOwner - cannot restore") + return { success: false, message: "Cannot rollback ownership transfer: missing previousOwner" } + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts` around lines 1458 - 1465, The rollback currently assumes edit.account was the previous owner, but ACL delegates can transfer on behalf of others; record the real previous owner when creating the transfer and use it for rollbacks. Update the transfer/ownership-change code path that sets token.owner (identify where ownership is changed and the edit is constructed) to capture oldOwner into edit.data.previousOwner before mutating token.owner, and modify the rollback branch in GCRTokenRoutines (the block using edit.isRollback, token.owner, edit.account) to set token.owner = edit.data.previousOwner (with a safe fallback to edit.account if previousOwner is missing) so rollbacks restore the actual prior owner.
🧹 Nitpick comments (5)
src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts (4)
1598-1621: Mutations not applied in simulate mode but success returned.When
simulate=trueand the script produces mutations, the function returnssuccess: truewithout applying them. This is correct for validation-only scenarios, but the response at line 1629 showsmutations: result.mutations.lengthwhich could be misleading since they weren't actually applied.Consider adding a note in the response that mutations were not persisted due to simulation mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts` around lines 1598 - 1621, The response currently reports the number of mutations even when simulate is true and they were not persisted; update the return payload in the function handling custom methods (check the simulate flag and the variables result.mutations and tokenData/applyGCRTokenDataToEntity) to include a clear note that mutations were only simulated and not persisted when simulate === true (e.g., add a field or append to the message indicating "simulated — mutations not applied/persisted"); ensure this conditional message is set in the same return path that currently returns success:true so callers can distinguish simulated vs persisted runs.
67-77: Singleton HookExecutor is not thread-safe for concurrent consensus routines.The static
hookExecutorsingleton is lazily initialized without synchronization. If multiple async paths callgetHookExecutor()concurrently before initialization completes, multiple instances could be created (though only the last assignment survives). More importantly, a single sharedHookExecutormay carry internal state that could leak across unrelated token operations if not designed for concurrent use.Consider initializing eagerly or verifying
HookExecutoris stateless/reentrant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts` around lines 67 - 77, The lazy static hookExecutor in GCRTokenRoutines (hookExecutor and getHookExecutor) is not safe for concurrent initialization and may leak state; replace the lazy-init with an eager, thread-safe initialization or add synchronization: either initialize hookExecutor at class load time using new HookExecutor(scriptExecutor) (ensuring scriptExecutor is available), or guard creation in getHookExecutor with a mutex/lock or double-checked async-safe pattern so only one HookExecutor is constructed, and audit HookExecutor for internal mutable state (make it stateless/reentrant or create per-call instances where needed).
947-984: Pause/unpause handlers lack transactional locking.Unlike transfer/mint/burn, the pause handlers use
findOneBy+savewithout a transaction and pessimistic lock. While pause is somewhat idempotent, concurrent operations could still cause unexpected interleaving. Consider adding transactional consistency for non-simulate paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts` around lines 947 - 984, The pause/unpause path in GCRTokenRoutines.handlePauseToken currently does a findOneBy + save without transactional locking; wrap the non-simulate write path in a database transaction and acquire a pessimistic write lock on the token row before mutating and saving it (use the repository's transaction/manager or a query builder with setLock("pessimistic_write") to re-load the token within the transaction), then update token.paused and save inside that transaction, preserving the simulate branch unchanged; reference GCRTokenRoutines.handlePauseToken, gcrTokenRepository, simulate, edit.isRollback, and the existing findOneBy/save calls when making the change.
1901-1936: Performance concern:getTokensOfloads all tokens into memory.As noted in the code comment, this is O(n) over all tokens. With a large token registry, this could cause memory pressure and slow responses. Since holder references are already maintained in
GCRMain.extended.tokens, consider adding an optimized path that queriesGCRMainfirst:// Optimized alternative using holder pointers const holder = await gcrMainRepository.findOneBy({ pubkey: holderAddress }) if (holder?.extended?.tokens) { return holder.extended.tokens.map(ref => ({ tokenAddress: ref.tokenAddress, ticker: ref.ticker, // ... would need to fetch current balance from token })) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts` around lines 1901 - 1936, getTokensOf currently loads all tokens (gcrTokenRepository.find()) which is O(n); instead first try to resolve holder pointers from GCRMain (e.g., fetch the holder record from gcrMainRepository and check holder.extended.tokens) and, if present, iterate those token references to fetch each token's current balance (fetch token by address or direct balance field) to build the result; only fall back to the existing full-scan over gcrTokenRepository.find() when the holder pointer list is absent, ensuring you avoid loading the entire token table when holder.extended.tokens is available.src/libs/consensus/v2/PoRBFT.ts (1)
778-780: Silent catch block hides fetch errors.The empty catch block at line 778 silently swallows all errors during peer tx fetching. While best-effort behavior is appropriate, consider logging at debug level to aid troubleshooting:
- } catch { - // best-effort; keep trying other peers - } + } catch (err) { + log.debug(`[CONSENSUS] Failed to fetch txs from peer: ${err}`) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/consensus/v2/PoRBFT.ts` around lines 778 - 780, The empty catch in the peer transaction fetch loop swallows errors; change it to catch (err) and emit a debug-level log rather than remaining silent so we preserve best-effort behavior while recording failures for troubleshooting—e.g., in the method/function in PoRBFT.ts where peer txs are fetched (the try/catch block around the peer fetch), replace the bare catch with catch (err) and call an existing debug logger (this.logger.debug or logger.debug) including the error object and contextual identifiers (peer id / tx id) so the fetch still continues on error but the failure is recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts`:
- Around line 192-204: getDeterministicBlockHeight currently falls back to 0
when neither tx.blockNumber nor getSharedState.lastBlockNumber is available,
which can produce non-deterministic behavior; change it to fail-fast by throwing
an error instead of returning 0. In getDeterministicBlockHeight (and using the
same pattern as getDeterministicTxTimestamp), after checking (tx as
any)?.blockNumber and getSharedState.lastBlockNumber and finding neither is a
valid non-negative number, throw a descriptive Error (including context like
"unable to resolve deterministic block height from transaction or shared state")
so callers cannot proceed with a risky default; keep references to Transaction
and getSharedState as the lookup sources.
- Around line 388-395: The current non-atomic check
(gcrTokenRepository.findOneBy({ address: tokenAddress })) followed by saving the
new token can race under concurrency; wrap the existence check and insert in a
single transaction (use the same transactional entity manager used elsewhere in
GCRTokenRoutines or a TypeORM transaction) and apply a pessimistic lock (e.g.,
SELECT ... FOR UPDATE / manager.findOne with lock: { mode: "pessimistic_write"
}) on the token/address row (or parent table row) before deciding to insert, or
alternatively perform an atomic upsert and handle unique-constraint errors from
create/save (catch and return the "already exists" result) so two concurrent
operations cannot both succeed; locate the logic around
gcrTokenRepository.findOneBy, the save call that creates the token, and
implement the transaction/locking there.
---
Duplicate comments:
In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts`:
- Around line 1351-1371: In the rollback branch (when edit.isRollback) you're
only restoring token.scriptVersion but not the prior script payload; update the
logic in the block that checks previousVersion to also read the prior script
from the edit payload (e.g., edit.data.previousScript) and set token.script =
edit.data.previousScript, token.hasScript = Boolean(edit.data.previousScript),
token.scriptVersion = previousVersion, and update token.lastScriptUpdate
appropriately (or preserve a provided previous timestamp if available like
edit.data.previousLastUpdate); keep the existing else branch that clears the
script when no previousVersion/info is provided.
- Around line 1458-1465: The rollback currently assumes edit.account was the
previous owner, but ACL delegates can transfer on behalf of others; record the
real previous owner when creating the transfer and use it for rollbacks. Update
the transfer/ownership-change code path that sets token.owner (identify where
ownership is changed and the edit is constructed) to capture oldOwner into
edit.data.previousOwner before mutating token.owner, and modify the rollback
branch in GCRTokenRoutines (the block using edit.isRollback, token.owner,
edit.account) to set token.owner = edit.data.previousOwner (with a safe fallback
to edit.account if previousOwner is missing) so rollbacks restore the actual
prior owner.
---
Nitpick comments:
In `@src/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.ts`:
- Around line 1598-1621: The response currently reports the number of mutations
even when simulate is true and they were not persisted; update the return
payload in the function handling custom methods (check the simulate flag and the
variables result.mutations and tokenData/applyGCRTokenDataToEntity) to include a
clear note that mutations were only simulated and not persisted when simulate
=== true (e.g., add a field or append to the message indicating "simulated —
mutations not applied/persisted"); ensure this conditional message is set in the
same return path that currently returns success:true so callers can distinguish
simulated vs persisted runs.
- Around line 67-77: The lazy static hookExecutor in GCRTokenRoutines
(hookExecutor and getHookExecutor) is not safe for concurrent initialization and
may leak state; replace the lazy-init with an eager, thread-safe initialization
or add synchronization: either initialize hookExecutor at class load time using
new HookExecutor(scriptExecutor) (ensuring scriptExecutor is available), or
guard creation in getHookExecutor with a mutex/lock or double-checked async-safe
pattern so only one HookExecutor is constructed, and audit HookExecutor for
internal mutable state (make it stateless/reentrant or create per-call instances
where needed).
- Around line 947-984: The pause/unpause path in
GCRTokenRoutines.handlePauseToken currently does a findOneBy + save without
transactional locking; wrap the non-simulate write path in a database
transaction and acquire a pessimistic write lock on the token row before
mutating and saving it (use the repository's transaction/manager or a query
builder with setLock("pessimistic_write") to re-load the token within the
transaction), then update token.paused and save inside that transaction,
preserving the simulate branch unchanged; reference
GCRTokenRoutines.handlePauseToken, gcrTokenRepository, simulate,
edit.isRollback, and the existing findOneBy/save calls when making the change.
- Around line 1901-1936: getTokensOf currently loads all tokens
(gcrTokenRepository.find()) which is O(n); instead first try to resolve holder
pointers from GCRMain (e.g., fetch the holder record from gcrMainRepository and
check holder.extended.tokens) and, if present, iterate those token references to
fetch each token's current balance (fetch token by address or direct balance
field) to build the result; only fall back to the existing full-scan over
gcrTokenRepository.find() when the holder pointer list is absent, ensuring you
avoid loading the entire token table when holder.extended.tokens is available.
In `@src/libs/consensus/v2/PoRBFT.ts`:
- Around line 778-780: The empty catch in the peer transaction fetch loop
swallows errors; change it to catch (err) and emit a debug-level log rather than
remaining silent so we preserve best-effort behavior while recording failures
for troubleshooting—e.g., in the method/function in PoRBFT.ts where peer txs are
fetched (the try/catch block around the peer fetch), replace the bare catch with
catch (err) and call an existing debug logger (this.logger.debug or
logger.debug) including the error object and contextual identifiers (peer id /
tx id) so the fetch still continues on error but the failure is recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70cb0e9d-25eb-4210-a9cc-8a201d9628a5
⛔ Files ignored due to path filters (1)
.mycelium/mycelium.dbis excluded by!**/*.db
📒 Files selected for processing (4)
.gitignoresrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/gcr/gcr_routines/GCRTokenRoutines.tssrc/libs/consensus/v2/PoRBFT.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libs/blockchain/chainBlocks.ts
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores