Fix reconcilation failure: improve detection of (in)effective "transfer value only" events#136
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
==========================================
+ Coverage 61.64% 61.65% +0.01%
==========================================
Files 48 48
Lines 3791 3792 +1
==========================================
+ Hits 2337 2338 +1
Misses 1302 1302
Partials 152 152 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andreibancioiu
changed the title
andreibancioiu
changed the title
| f"--first-historical-epoch={current_epoch}", | ||
| f"--num-historical-epochs={configuration.num_historical_epochs}", | ||
| f"--activation-epoch-spica={configuration.activation_epoch_spica}", | ||
| "--log-level=*:DEBUG", |
There was a problem hiding this comment.
Intended (explicit logging for our system tests).
| var ( | ||
| // RosettaMiddlewareVersion is the version of this package (application) | ||
| RosettaMiddlewareVersion = "v0.7.0" | ||
| RosettaMiddlewareVersion = "v0.7.1" |
There was a problem hiding this comment.
Hotfix (patch).
| } | ||
|
|
||
| if string(event.Data) != transactionEventDataExecuteOnDestContext && string(event.Data) != transactionEventDataAsyncCall { | ||
| if string(event.Data) != transactionEventDataExecuteOnDestContext && string(event.Data) != transactionEventDataAsyncCall && string(event.Data) != transactionEventTransferAndExecute { |
There was a problem hiding this comment.
We don't need a loop (yet!), I think.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes reconciliation failures by improving how “transfer value only” events are detected, particularly for TransferAndExecute contexts, and updates related versioning and tests.
- Bumped RosettaMiddlewareVersion to v0.7.1.
- Extended
decideEffectiveEventTransferValueOnlyAfterSiriusto treatTransferAndExecuteevents as effective and added the corresponding constant. - Added a unit test for intra-shard
TransferAndExecuteevents and enabled DEBUG logging in system tests.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| version/constants.go | Updated middleware version to v0.7.1. |
| systemtests/check_with_mesh_cli.py | Enabled --log-level=*:DEBUG for mesh CLI checks. |
| server/services/constants.go | Converted event var blocks to const, added TransferAndExecute. |
| server/services/transactionEventsController.go | Included TransferAndExecute in effective event logic. |
| server/services/transactionEventsController_test.go | Added test for intra-shard TransferAndExecute events. |
Comments suppressed due to low confidence (1)
server/services/constants.go:51
- [nitpick] The constant name
transactionEventTransferAndExecuteis inconsistent with the existingtransactionEventData*constants. Consider renaming it totransactionEventDataTransferAndExecutefor naming consistency.
transactionEventTransferAndExecute = "TransferAndExecute"
| topic0, | ||
| topic1, | ||
| }, | ||
| Data: []byte("TransferAndExecute"), |
There was a problem hiding this comment.
Hardcoding the event name string here risks a mismatch if the constant changes. Consider using the transactionEventTransferAndExecute constant instead of the literal.
There was a problem hiding this comment.
Bad (very bad, ~extremely bad) advice. It's important to have a failing test in such circumstances (e.g. mistakenly changing the constant in the production code).
3 participants
Original reconciliation failure:
The
transfer value onlyevent with value0.024625648702188092was previously ignored - mistakenly thought as being ineffective, in the functiondecideEffectiveEventTransferValueOnlyAfterSirius.We've changed the logic, so that
transfer value onlyevents emitted withinExecuteOnDestContext,AsyncCallandTransferAndExecuteare properly considered (previously, the latter wasn't).