Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/main/java/org/tron/core/consensus/ProposalService.java">
<violation number="1" location="framework/src/main/java/org/tron/core/consensus/ProposalService.java:101">
P3: The CLOSE_EXCHANGE apply case lacks the VERSION_CLOSE_EXCHANGE fork check that the two sibling skip paths added here both use, even though this case exists specifically as a guard against crafted/historical capsules. Such a capsule applied pre-fork would still advance the level to 1. Guard the apply with `manager.getChainBaseManager().getForkController().pass(ForkBlockVersionEnum.VERSION_CLOSE_EXCHANGE)` for consistency with the create-side gate.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.java:835">
P2: `testCloseExchangeProposal` leaves the shared test context mutated: `CLOSE_EXCHANGE` is persisted at terminal value 2 and the VERSION_CLOSE_EXCHANGE fork is left activated. Every other test in this batch (ProposalServiceTest, ProposalCreateActuatorTest) restores the fork state in a `finally` block; only this method has no cleanup. Any later test in the same class/Spring context that reads `getCloseExchange()` or validates EXCHANGE_CREATE_FEE/ALLOW_HARDEN_EXCHANGE_CALCULATION will see stale state (fork passed + level 2). Reset the level and clear the fork stats at the end of the method (e.g. `dynamicPropertiesStore.saveCloseExchange(0)` plus a `deactivateCloseExchangeFork()`), or share the cleanup helper used by the other two classes.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/WalletTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/WalletTest.java:1179">
P3: This test calls saveCloseExchange(2) on the class-shared database and never restores it. WalletTest extends BaseTest, which keeps one Application context for every @Test method in the class, and before() does not reset this property, so all later tests in this class run with exchange close level 2. Currently nothing in the file breaks, but any current or future exchange-actuator test running after this one will be rejected (Withdraw at level 2), and test order in JUnit4 is an implicit, deterministic-but-arbitrary contract. Reset with saveCloseExchange(0) at the end of the test, matching how other tests in the file restore properties (e.g. saveAllowSameTokenName(0)).</violation>
</file>
<file name="framework/src/test/java/org/tron/core/services/ProposalServiceTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/services/ProposalServiceTest.java:277">
P3: This test leaves `closeExchange=1` and `exchangeCreateFee=2_048_000_000` in the shared store: the final `apply(CLOSE_EXCHANGE_CODE, 1)` and the post-fork skip leave those values behind, while the `finally` block restores only the fork state and the harden flag. BaseTest explicitly keeps all @Test methods in the class on one shared context, so later tests (including the pre-existing `test()`, which replays every ProposalType with value 1) see this residue. Reset the two stored values in the `finally` block to the baseline captured at the start of the test.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/db/CloseExchangeGateTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/db/CloseExchangeGateTest.java:149">
P3: closeExchangeDefaultsToZeroAndRoundTrips never exercises the "missing key -> 0" default it claims to cover: @Before initTest() unconditionally writes saveCloseExchange(0), so the key always exists when the assertion runs. A regression that made getCloseExchange() return nonzero on a missing key would pass this test. Delete the key explicitly to test the default, or adjust the javadoc to state this only verifies the explicit-save round trip.</violation>
<violation number="2" location="framework/src/test/java/org/tron/core/db/CloseExchangeGateTest.java:191">
P3: This comment is stale: with NAME_ASCENDING ordering the generateBlock* tests run before this one, and createExchangeOnChain() has already persisted exchange id 1, so the ExchangeWithdraw fixture here does have an on-chain exchange and can execute rather than "fail in its own doValidate". The assertion still holds, but the comment misstates the fixture state and the test is implicitly order-dependent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| ProposalCapsule proposalCapsule = new ProposalCapsule( | ||
| Protocol.Proposal.newBuilder().putParameters(code, 1).build()); | ||
| ProposalService.process(dbManager, proposalCapsule); |
There was a problem hiding this comment.
P2: testCloseExchangeProposal leaves the shared test context mutated: CLOSE_EXCHANGE is persisted at terminal value 2 and the VERSION_CLOSE_EXCHANGE fork is left activated. Every other test in this batch (ProposalServiceTest, ProposalCreateActuatorTest) restores the fork state in a finally block; only this method has no cleanup. Any later test in the same class/Spring context that reads getCloseExchange() or validates EXCHANGE_CREATE_FEE/ALLOW_HARDEN_EXCHANGE_CALCULATION will see stale state (fork passed + level 2). Reset the level and clear the fork stats at the end of the method (e.g. dynamicPropertiesStore.saveCloseExchange(0) plus a deactivateCloseExchangeFork()), or share the cleanup helper used by the other two classes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.java, line 835:
<comment>`testCloseExchangeProposal` leaves the shared test context mutated: `CLOSE_EXCHANGE` is persisted at terminal value 2 and the VERSION_CLOSE_EXCHANGE fork is left activated. Every other test in this batch (ProposalServiceTest, ProposalCreateActuatorTest) restores the fork state in a `finally` block; only this method has no cleanup. Any later test in the same class/Spring context that reads `getCloseExchange()` or validates EXCHANGE_CREATE_FEE/ALLOW_HARDEN_EXCHANGE_CALCULATION will see stale state (fork passed + level 2). Reset the level and clear the fork stats at the end of the method (e.g. `dynamicPropertiesStore.saveCloseExchange(0)` plus a `deactivateCloseExchangeFork()`), or share the cleanup helper used by the other two classes.</comment>
<file context>
@@ -744,6 +747,125 @@ private void testAllowHardenExchangeCalculationProposal() {
+ }
+ ProposalCapsule proposalCapsule = new ProposalCapsule(
+ Protocol.Proposal.newBuilder().putParameters(code, 1).build());
+ ProposalService.process(dbManager, proposalCapsule);
+
+ thrown = assertThrows(ContractValidateException.class,
</file context>
| .findFirst() | ||
| .orElse(null); | ||
| Assert.assertNotNull("getCloseExchange chain parameter must be exposed", closeExchange); | ||
| Assert.assertEquals(2L, closeExchange.getValue()); |
There was a problem hiding this comment.
P3: This test calls saveCloseExchange(2) on the class-shared database and never restores it. WalletTest extends BaseTest, which keeps one Application context for every @test method in the class, and before() does not reset this property, so all later tests in this class run with exchange close level 2. Currently nothing in the file breaks, but any current or future exchange-actuator test running after this one will be rejected (Withdraw at level 2), and test order in JUnit4 is an implicit, deterministic-but-arbitrary contract. Reset with saveCloseExchange(0) at the end of the test, matching how other tests in the file restore properties (e.g. saveAllowSameTokenName(0)).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/WalletTest.java, line 1179:
<comment>This test calls saveCloseExchange(2) on the class-shared database and never restores it. WalletTest extends BaseTest, which keeps one Application context for every @Test method in the class, and before() does not reset this property, so all later tests in this class run with exchange close level 2. Currently nothing in the file breaks, but any current or future exchange-actuator test running after this one will be rejected (Withdraw at level 2), and test order in JUnit4 is an implicit, deterministic-but-arbitrary contract. Reset with saveCloseExchange(0) at the end of the test, matching how other tests in the file restore properties (e.g. saveAllowSameTokenName(0)).</comment>
<file context>
@@ -1162,6 +1162,23 @@ public void testGetChainParameters() {
+ .findFirst()
+ .orElse(null);
+ Assert.assertNotNull("getCloseExchange chain parameter must be exposed", closeExchange);
+ Assert.assertEquals(2L, closeExchange.getValue());
+ }
+
</file context>
| Assert.assertEquals(2L, closeExchange.getValue()); | |
| Assert.assertEquals(2L, closeExchange.getValue()); | |
| dbManager.getDynamicPropertiesStore().saveCloseExchange(0); |
| } finally { | ||
| deactivateCloseExchangeFork(); | ||
| dps().saveAllowHardenExchangeCalculation(0); |
There was a problem hiding this comment.
P3: This test leaves closeExchange=1 and exchangeCreateFee=2_048_000_000 in the shared store: the final apply(CLOSE_EXCHANGE_CODE, 1) and the post-fork skip leave those values behind, while the finally block restores only the fork state and the harden flag. BaseTest explicitly keeps all @test methods in the class on one shared context, so later tests (including the pre-existing test(), which replays every ProposalType with value 1) see this residue. Reset the two stored values in the finally block to the baseline captured at the start of the test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/services/ProposalServiceTest.java, line 277:
<comment>This test leaves `closeExchange=1` and `exchangeCreateFee=2_048_000_000` in the shared store: the final `apply(CLOSE_EXCHANGE_CODE, 1)` and the post-fork skip leave those values behind, while the `finally` block restores only the fork state and the harden flag. BaseTest explicitly keeps all @Test methods in the class on one shared context, so later tests (including the pre-existing `test()`, which replays every ProposalType with value 1) see this residue. Reset the two stored values in the `finally` block to the baseline captured at the start of the test.</comment>
<file context>
@@ -151,4 +154,152 @@ public void testProposalExpireTime() {
+ dps().saveCloseExchange(0);
+ apply(CLOSE_EXCHANGE_CODE, 1);
+ Assert.assertEquals(1, dps().getCloseExchange());
+ } finally {
+ deactivateCloseExchangeFork();
+ dps().saveAllowHardenExchangeCalculation(0);
</file context>
| } finally { | |
| deactivateCloseExchangeFork(); | |
| dps().saveAllowHardenExchangeCalculation(0); | |
| } finally { | |
| deactivateCloseExchangeFork(); | |
| dps().saveAllowHardenExchangeCalculation(0); | |
| dps().saveExchangeCreateFee(1024_000_000L); | |
| dps().saveCloseExchange(0); | |
| } |
|
|
||
| // Withdraw requires level 2, so it passes the gate at level 1 (its remaining | ||
| // doValidate checks are out of scope here). | ||
| assertFalse("Withdraw must pass the actuator gate at level 1", |
There was a problem hiding this comment.
P3: This comment is stale: with NAME_ASCENDING ordering the generateBlock* tests run before this one, and createExchangeOnChain() has already persisted exchange id 1, so the ExchangeWithdraw fixture here does have an on-chain exchange and can execute rather than "fail in its own doValidate". The assertion still holds, but the comment misstates the fixture state and the test is implicitly order-dependent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/db/CloseExchangeGateTest.java, line 191:
<comment>This comment is stale: with NAME_ASCENDING ordering the generateBlock* tests run before this one, and createExchangeOnChain() has already persisted exchange id 1, so the ExchangeWithdraw fixture here does have an on-chain exchange and can execute rather than "fail in its own doValidate". The assertion still holds, but the comment misstates the fixture state and the test is implicitly order-dependent.</comment>
<file context>
@@ -0,0 +1,704 @@
+
+ // Withdraw requires level 2, so it passes the gate at level 1 (its remaining
+ // doValidate checks are out of scope here).
+ assertFalse("Withdraw must pass the actuator gate at level 1",
+ buildPhaseRejectionContainsCloseLevel(ContractType.ExchangeWithdrawContract, 1));
+ }
</file context>
|
|
||
| @Test | ||
| public void closeExchangeDefaultsToZeroAndRoundTrips() { | ||
| assertEquals(0, dbManager.getDynamicPropertiesStore().getCloseExchange()); |
There was a problem hiding this comment.
P3: closeExchangeDefaultsToZeroAndRoundTrips never exercises the "missing key -> 0" default it claims to cover: @before initTest() unconditionally writes saveCloseExchange(0), so the key always exists when the assertion runs. A regression that made getCloseExchange() return nonzero on a missing key would pass this test. Delete the key explicitly to test the default, or adjust the javadoc to state this only verifies the explicit-save round trip.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/db/CloseExchangeGateTest.java, line 149:
<comment>closeExchangeDefaultsToZeroAndRoundTrips never exercises the "missing key -> 0" default it claims to cover: @Before initTest() unconditionally writes saveCloseExchange(0), so the key always exists when the assertion runs. A regression that made getCloseExchange() return nonzero on a missing key would pass this test. Delete the key explicitly to test the default, or adjust the javadoc to state this only verifies the explicit-save round trip.</comment>
<file context>
@@ -0,0 +1,704 @@
+
+ @Test
+ public void closeExchangeDefaultsToZeroAndRoundTrips() {
+ assertEquals(0, dbManager.getDynamicPropertiesStore().getCloseExchange());
+ assertFalse(dbManager.getDynamicPropertiesStore().allowHardenExchangeCalculation());
+
</file context>
| manager.getDynamicPropertiesStore().saveExchangeCreateFee(entry.getValue()); | ||
| break; | ||
| } | ||
| case CLOSE_EXCHANGE: { |
There was a problem hiding this comment.
P3: The CLOSE_EXCHANGE apply case lacks the VERSION_CLOSE_EXCHANGE fork check that the two sibling skip paths added here both use, even though this case exists specifically as a guard against crafted/historical capsules. Such a capsule applied pre-fork would still advance the level to 1. Guard the apply with manager.getChainBaseManager().getForkController().pass(ForkBlockVersionEnum.VERSION_CLOSE_EXCHANGE) for consistency with the create-side gate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/consensus/ProposalService.java, line 101:
<comment>The CLOSE_EXCHANGE apply case lacks the VERSION_CLOSE_EXCHANGE fork check that the two sibling skip paths added here both use, even though this case exists specifically as a guard against crafted/historical capsules. Such a capsule applied pre-fork would still advance the level to 1. Guard the apply with `manager.getChainBaseManager().getForkController().pass(ForkBlockVersionEnum.VERSION_CLOSE_EXCHANGE)` for consistency with the create-side gate.</comment>
<file context>
@@ -89,9 +89,25 @@ public static boolean process(Manager manager, ProposalCapsule proposalCapsule)
manager.getDynamicPropertiesStore().saveExchangeCreateFee(entry.getValue());
break;
}
+ case CLOSE_EXCHANGE: {
+ int current = manager.getDynamicPropertiesStore().getCloseExchange();
+ // Defense-in-depth: only apply the next step of the graded {0,1,2} ladder.
</file context>
71c7b64 to
2fbf6e7
Compare
- remove Manager three-entrance wiring and helper methods; keep the existing VERSION_4_8_0_1 swap gate as the Manager-side baseline - reject ExchangeCreate/Inject/Transaction at level >= 1 and ExchangeWithdraw at level == 2 in each actuator's validate(); error message format unchanged - Wallet.createTransactionCapsule pre-validation surfaces the rejection at build time - rewrite CloseExchangeGateTest as actuator-layer truth table and fix ProposalUtilTest assertions
… coverage, wallet param order)
2fbf6e7 to
0df5b21
Compare
eaef95c to
9a4aed5
Compare
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/test/java/org/tron/core/services/ProposalServiceTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/services/ProposalServiceTest.java:275">
P3: The comment says the test applies CLOSE_EXCHANGE "at close level >= 1", but the level is reset to 0 immediately before applying value 1, so this only re-checks a fresh 0→1 advance. To genuinely exercise the level >= 1 regime, keep the level at 1 and apply value 2 (e.g. `dps().saveCloseExchange(1); apply(CLOSE_EXCHANGE_CODE, 2); Assert.assertEquals(2, dps().getCloseExchange());`), or reword the comment to match what it tests.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| Assert.assertEquals(1, dps().getAllowHardenExchangeCalculation()); | ||
| Assert.assertEquals(5000L, dps().getCreateAccountFee()); | ||
|
|
||
| // CLOSE_EXCHANGE application itself still works at close level >= 1 |
There was a problem hiding this comment.
P3: The comment says the test applies CLOSE_EXCHANGE "at close level >= 1", but the level is reset to 0 immediately before applying value 1, so this only re-checks a fresh 0→1 advance. To genuinely exercise the level >= 1 regime, keep the level at 1 and apply value 2 (e.g. dps().saveCloseExchange(1); apply(CLOSE_EXCHANGE_CODE, 2); Assert.assertEquals(2, dps().getCloseExchange());), or reword the comment to match what it tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/services/ProposalServiceTest.java, line 275:
<comment>The comment says the test applies CLOSE_EXCHANGE "at close level >= 1", but the level is reset to 0 immediately before applying value 1, so this only re-checks a fresh 0→1 advance. To genuinely exercise the level >= 1 regime, keep the level at 1 and apply value 2 (e.g. `dps().saveCloseExchange(1); apply(CLOSE_EXCHANGE_CODE, 2); Assert.assertEquals(2, dps().getCloseExchange());`), or reword the comment to match what it tests.</comment>
<file context>
@@ -270,36 +272,14 @@ public class ProposalServiceTest extends BaseTest {
Assert.assertEquals(5000L, dps().getCreateAccountFee());
- // CLOSE_EXCHANGE application itself still works after the fork
+ // CLOSE_EXCHANGE application itself still works at close level >= 1
dps().saveCloseExchange(0);
apply(CLOSE_EXCHANGE_CODE, 1);
</file context>
| // CLOSE_EXCHANGE application itself still works at close level >= 1 | |
| // CLOSE_EXCHANGE application itself still works while the close-level gating is active |
…GY_FEE fall-through
…,version code:18830 (tronprotocol#7008)
tronprotocol#6982) * refactor(api): replace response copy wrappers with Jetty content count * fix(api): preserve legacy Unicode replacement in JsonFormat Use JDK UTF-8 conversion to replace malformed surrogates in parser error descriptions and isolated low surrogates in string output. Match the legacy writer's question-mark replacement while preserving valid Unicode and parsing behavior. Cover identifier, ABI escape, numeric errors, and low-surrogate output with regression tests, including strict UTF-8 checks through native Jetty responses.
5 participants
What does this PR do?
Implements TIP-937, retiring the built-in Bancor trading contracts (ExchangeCreate / ExchangeInject / ExchangeTransaction / ExchangeWithdraw) in irreversible, stepwise levels.
CLOSE_EXCHANGE(code 100; upstream code 99 is alreadyALLOW_OPTIMIZE_TVM_STORAGE), gated by the newForkBlockVersionEnum.VERSION_4_8_3(value 39; upstream'sVERSION_4_8_2_3remains 38 andBLOCK_VERSIONis not bumped by this feature). Value semantics: 0 = default (nothing closed), 1 = Create/Inject/Transaction closed, 2 = Withdraw additionally closed. Proposal creation is fork-gated and advances the level by exactly +1 within [1,2] (irreversible by design); same, skip, and lower levels are rejected, level 2 is terminal, and mixed-parameter proposals are allowed (there is no single-parameter requirement). Once the level is >= 1, the legacy parametersEXCHANGE_CREATE_FEE(12) andALLOW_HARDEN_EXCHANGE_CALCULATION(98) are locked — rejected at proposal creation, skipped on apply with the proposal's remaining parameters still processed, and stored values preserved. This lockout is tied to the close level only, not to fork activation, so those parameters stay proposable at close level 0, subject to their existing fork/value rules.validate()of the four exchange actuators, checked after the existing null guards — Create/Inject/Transaction reject at level >= 1, Withdraw at level == 2 — with the message<ContractType> is rejected by exchange close level <level>. There is no separate CLOSE_EXCHANGE check inManager; generic packing simply drops transactions that fail validation, and build-time prevalidation, broadcast, packing, and block execution all pass through this validate path. At level 0 the new close checks are a no-op and existing exchange transaction validation is preserved, including the legacy ExchangeTransaction rejection (which can fire earlier when harden = 0 and keeps its original message). Level 0 is not byte-identical to current mainnet, since the new chain parametergetCloseExchangebecomes visible viagetChainParameters.Parameter.java/Wallet: adds theVERSION_4_8_3fork version (value 39, with 80% fork voting support required for activation) and exposes the current close level as agetChainParametersentry under the keygetCloseExchange(no new standalone API);CLOSE_EXCHANGEdefaults to 0 and persists through the standard store. Ordinary production runningBLOCK_VERSION38 does not activate fork 39 — activation requires a coordinated release advertising 39 and passing fork voting. Existing read-only exchange queries (getexchangebyid,listexchanges) are unchanged, and existing balances are neither migrated nor erased. At level 1 creators retain ExchangeWithdraw on their pools; at level 2 withdrawals are unavailable.Why are these changes required?
Per TIP-937's motivating research, the Bancor exchange feature is dormant: 186 trading pairs, the latest created on 2024-04-19, a ~20-month scan showing zero calls of the four contract types, and roughly 940,286 TRX sitting in pools controlled solely by pair creators (historical figures from the TIP, not a newly verified snapshot). ExchangeTransaction is already rejected on mainnet via the legacy harden-parameter path. The stepwise one-way close lets creators withdraw funds at level 1 while the committee decides, through separate proposals, whether to proceed to level 2. See TIP-937 for full motivation and rationale.
This PR has been tested by:
WalletTest.getTransactionById). The four existing Exchange actuator suites run as level-0 regression, with separateCloseExchangeGateintegration cases across build, broadcast, packing, and block execution, alongsideProposalUtil,ProposalCreate,ProposalService,Wallet, andOptimizeTvmStorage; this is not an exhaustive per-actuator/per-pathway matrix. Targeted tests only; no full CI or RocksDB run, and end-to-end fork activation of version 39 was not exercised here.-x generateGitPropertiesdue to the linked worktree's JGit limitation).Follow up
VERSION_4_8_3) ships in this release, but ordinary production nodes onBLOCK_VERSION38 will not activate it; a coordinated activation release must advertise 39 and pass 80% fork voting. Actual closure levels are then set by separate committee proposals after the fork passes.Extra details
release_v4.8.3at 6f7b83a; the feature delta versus upstream is 14 files (+1233/−3). This fork PR's baserelease_v4.8.3(4d6c240) is older than that merge, so GitHub additionally shows the included upstream changes; the base is unchanged.