Bump to compact 0.28.0 #62
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis pull request upgrades the Compact toolchain from version 0.26.0 to 0.28.0 and updates the compact-runtime dependency to 0.14.0-rc.0. The changes include refactoring the simulator core to use new runtime types (StateValue, ChargedState), replacing the CircuitContext structure to use currentQueryContext and costModel instead of originalState and transactionContext, migrating artifact imports from CommonJS to ES modules, and updating Compact contract files to language version 0.19.0 with named imports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/setup/action.yml (1)
42-46: Updatecompact-versionto an available release; version 0.28.0 does not exist.The latest released Compact compiler version is
0.26.0(October 8, 2025). Version0.28.0is not a published release and will cause the setup action to fail. Update to0.26.0or verify the correct version number.
🧹 Nitpick comments (2)
packages/cli/README.md (1)
238-244: Align remaining example versions for consistency.This output block shows 0.28.0, but earlier examples still reference 0.26.0 (e.g.,
+0.26.0and programmatic API snippets). Consider updating those to reduce confusion.packages/simulator/test/unit/core/StateManager.test.ts (1)
99-133: Avoid usingQueryContextas thecostModelplaceholder.
costModelis currently set tomodifiedTxCtx(aQueryContext). If the runtime expects a distinct cost model type, this can mask mismatches. Consider reusing the existing context’scostModel(or constructing a proper one) instead.Suggested tweak
const newCtx: CircuitContext<SimplePrivateState> = { currentQueryContext: modifiedTxCtx, currentPrivateState: initialPrivateState, currentZswapLocalState: zswapLocalState_1, - costModel: modifiedTxCtx, + costModel: ctx.costModel, };
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
Thank you @andrew-fleming! Looking good! I left some comments.
packages/simulator/test/fixtures/sample-contracts/SampleZOwnable.compact
Outdated
Show resolved
Hide resolved
packages/simulator/test/fixtures/sample-contracts/Simple.compact
Outdated
Show resolved
Hide resolved
packages/simulator/test/fixtures/sample-contracts/Witness.compact
Outdated
Show resolved
Hide resolved
emnul
left a comment
emnul
left a comment
There was a problem hiding this comment.
Looks good! I just have an open question about ledger v7
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
Thank you @andrew-fleming! Changes look good! Left some comments mainly regarding gasLimit is it excluded as we are dealing with the Simulator so the gasLimit here is useless?
| */ | ||
| public getCallerContext(): CircuitContext<P> { | ||
| const activeCaller = this.callerOverride || this.persistentCallerOverride; | ||
| const baseCtx = this.circuitContext; |
There was a problem hiding this comment.
Why do we need to wrap this.circuitContext in another variable?
There was a problem hiding this comment.
We don't need to wrap it. It simplifies the return object so we're not rewriting this.circuitContext multiple times
return {
currentPrivateState: baseCtx.currentPrivateState,
currentQueryContext: baseCtx.currentQueryContext,
currentZswapLocalState: activeCaller
? emptyZswapLocalState(activeCaller)
: baseCtx.currentZswapLocalState,
costModel: baseCtx.costModel,
};
| const baseCtx = this.circuitContext; | ||
|
|
||
| return { | ||
| ...this.circuitContext, |
There was a problem hiding this comment.
Why do we not return the rest of circuitContext here? we are basically excluding gasLimit is that intended?
If that is not intended, I would revert that as returning gasLimit is useful.
There was a problem hiding this comment.
It is a part of the circuit context interface and it's better for future proofing. Will add it in 👍
| contractAddress, | ||
| ), | ||
| currentQueryContext: new QueryContext(chargedState, contractAddress), | ||
| costModel: CostModel.initialCostModel(), |
There was a problem hiding this comment.
| costModel: CostModel.initialCostModel(), | |
| costModel: CostModel.initialCostModel(), | |
| gasLimit: emptyRunningCost() |
I think we can use this API for the default value: https://github.com/midnightntwrk/compact-export/blob/41de8595c568de8dfafc9e80bec60630c5417b00/runtime/src/circuit-context.ts#L159
There was a problem hiding this comment.
All contract tests that change the state:
CompactError: Error: ran out of gas budget
| public getContractState(): ContractState { | ||
| return this.circuitContext.originalState; | ||
| public getContractState(): StateValue { | ||
| return this.circuitContext.currentQueryContext.state.state; |
There was a problem hiding this comment.
I don't like the naming of the chargedState here, it should be rather currentQueryContext.chargedState.state. If we agree I will write it as a small feedback to the MN team.
| contractAddress, | ||
| ), | ||
| currentZswapLocalState: emptyZswapLocalState(sender), | ||
| costModel: currentContext.costModel, |
There was a problem hiding this comment.
Do we need to return the gasLimit here?
There was a problem hiding this comment.
Technically no bc it's an optional param. Will add for future proofing
| }); | ||
| export const encodeToAddress = (str: string): Compact.ContractAddress => { | ||
| const hex = toHexPadded(str); | ||
| // Remove last 2 bytes (4 hex chars) to comply with 31-byte field limit |
There was a problem hiding this comment.
| // Remove last 2 bytes (4 hex chars) to comply with 31-byte field limit | |
| // Remove last 2 bytes (4 hex chars) to comply with 32-byte field limit |
Correct me if I'm wrong, but I think that's what do you mean.
There was a problem hiding this comment.
Indeed, thus it's not needed. Removing
Co-authored-by: 0xisk <0xisk@proton.me> Signed-off-by: Andrew Fleming <fleming.andrew@protonmail.com>
3 participants
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #59.
Note that the root README badge still shows 0.26.0. Instead of changing it now, this PR proposes to wait until there's a release document for it to point to. This PR could change the badge's version number and keep the link if preferred (but it's easier to forget if the output if correct)
This PR also proposes not to update all the CLI tests that use 0.26.0 as an input value. It's simple to change if it's really desired
PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.