Declaring preimages and lookup history #101
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
📝 WalkthroughWalkthroughThis PR extends the service deployment infrastructure to support preimages and lookup_history. New optional fields are added to the deployment configuration, validated with schemas, passed through the generateServiceOutput function, and processed during genesis state generation to provision preimage data and update lookup histories. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 Fix all issues with AI agents
In `@bin/cli/utils/config-validator.test.ts`:
- Around line 785-810: Update the test so it expects validation to reject
negative slot values instead of accepting them: use validateBuildConfig (the
function under test) to assert it returns an error or throws for the provided
config containing negative lookup_history slots (i.e., ensure the validation
fails and surfaces an error mentioning lookup_history/Slot out-of-range),
referencing validateBuildConfig and the Slot validation rule so the test
confirms negative numbers are invalid.
In `@bin/cli/utils/config-validator.ts`:
- Around line 38-42: The lookup_history property in
ServiceDeploymentConfigSchema currently accepts any integers but must match
Slot() constraints (unsigned 32-bit) to avoid runtime crashes; update the
lookup_history record value schema (the z.array(...) for lookup_history) to only
allow integers that are integer, >= 0 and <= 0xffffffff and still enforce max(3)
per array so invalid negative or too-large slot values are rejected at
validation time (update the schema expression referenced by
ServiceDeploymentConfigSchema and the lookup_history key; no other behavioral
changes).
🧹 Nitpick comments (3)
packages/jammin-sdk/util/generate-service-output.ts (1)
25-41: Add JSDoc for the public API signature (new params).
This function is exported and now has new parameters; adding JSDoc keeps the public surface documented.✍️ Suggested JSDoc
+/** + * Build service output from a JAM file and optional deployment overrides. + * `@param` jamFilePath Path to the JAM file. + * `@param` serviceId Numeric service identifier (u32). + * `@param` storage Optional storage key/value pairs. + * `@param` info Optional service account info overrides. + * `@param` preimages Optional map of preimage hashes to blobs. + * `@param` lookupHistory Optional map of preimage hashes to lookup history slots. + */ export async function generateServiceOutput(As per coding guidelines, Use JSDoc comments for public APIs (functions, classes, exported types).
packages/jammin-sdk/genesis-state-generator.test.ts (2)
151-167: Strengthen preimage and lookup history test assertions.These tests verify that
generateStatedoesn't crash and creates a service account, but they don't actually verify that the preimages are stored and retrievable from the service. Since preimages and lookup history are the main feature of this PR, consider adding assertions that verify the data is actually persisted.For example, after generating state, retrieve the preimage from the service account and compare it to the expected value.
💡 Suggested enhancement for stronger assertions
const state = generateState([serviceWithPreimages]); expect(state).toBeDefined(); const serviceAccount = state.services.get(ServiceId(300)); expect(serviceAccount).toBeDefined(); + + // Verify preimage is actually stored and retrievable + const storedPreimage = serviceAccount?.getPreimage(preimageHash, preimageBlob.length); + expect(storedPreimage?.toString()).toBe(preimageBlob.toString()); });Similarly for the multiple slots test - verify the lookup history entries are accessible.
Also applies to: 169-185
29-29: Remove unnecessaryasynckeywords from test functions.Multiple test functions are marked as
asyncbut don't contain anyawaitexpressions. While not harmful, this adds unnecessary overhead and could be misleading to readers.♻️ Example fix for one test
- test("should generate genesis with basic services", async () => { + test("should generate genesis with basic services", () => {Apply similarly to other affected tests.
Also applies to: 49-49, 59-59, 91-91, 112-112, 129-129, 151-151, 169-169, 187-187
tomusdrw
left a comment
tomusdrw
left a comment
There was a problem hiding this comment.
lgtm! I would just consider aligning the format with some existing json vectors from w3f, but afaik these has recently changed
|
@tomusdrw do you think we should increment storage utilisation for these preimages? |
|
what if our preimage hash is not a hash of a preimage, aka. our key to preimage is incorrect? Maybe we shoul provide preimages by simply giving preimageBlob and its availability, and it creates correct keys internally? |
if storage is not given by the user - yes, it should be calculated from these storage items. otherwise - it's up to the user (plus the warning that we've discussed in another pr). |
good point. just providing blobs would be more friendly, but the question then is - how do we know what the hashes are and what lookup items are needed? (btw, we should probably also suggest to the user that a preimage blob needs valid lookup history slots as well). Providing "fake" blobs for some hashes is useful, but I think it might be better suited as some sort of "override" configuration where we know we explicitly want to break some consistency assumptions. Thinking about it now, having custom storage count/bytes falls into the same category. I'll leave the decision to you guys, I think it's fine either way, and we yet have to figure out what use cases are there and what kind of format brings the most value. |
|
We are building state by using const blob = BytesBlob.parseBlob("0x18000f4e554c4c20417574686f72697a65720131034343300000000000000000000000000a00000000000633073308320015");
const hash = blake2b.hashBytes(blob).asOpaque();
const preimage = PreimageItem.create({ hash, blob });
const preimageLookupHistory = new LookupHistoryItem(
hash,
U32(blob.raw.length),
tryAsLookupHistorySlots([Slot(1), Slot(2), Slot(3)]),
);
update.preimages?.set(serviceId, [
UpdatePreimage.provide({
preimage,
slot: Slot(0),
providedFor: serviceId,
}),
UpdatePreimage.updateOrAdd({ lookupHistory: preimageLookupHistory }),
]);so IMO we could simply provide preimages like deployment:
spawn: local
services:
jambrains:
preimages:
"0x18000f4e554c4c20417574686f72697a65720131034343300000000000000000000000000a00000000000633073308320015" : [1, 2, 3]and even the lookup history can simply default to [0], so in preimages we could also provide just a blob, without |
this won't work:
besides, dropping the hash limits debugging possibilites for the user. some that come to mind:
given the nature of this project i think keeping extra debugging flexibility is a worthy goal. for the time being i dont see any convincing arguments for change or even a format that would actually work so will keep the current format, plus new naming. future changes are very welcome if it turns out there are practical benefits to the user. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/jammin-sdk/genesis-state-generator.test.ts`:
- Around line 2-10: The import for ServiceBuildOutput is using the wrong
relative directory; update the import in genesis-state-generator.test.ts to
import ServiceBuildOutput from "./utils/generate-service-output" (replace
"./util/generate-service-output" with "./utils/generate-service-output") so the
ServiceBuildOutput type resolves correctly when running tests that use
generateGenesis, generateState, and toJip4Schema.
In `@packages/jammin-sdk/genesis-state-generator.ts`:
- Around line 152-170: The storage counting currently adds
LOOKUP_HISTORY_ENTRY_BYTES and the preimageBlob length and increments
calculatedStorageCount by 2 unconditionally; change it so
LOOKUP_HISTORY_ENTRY_BYTES (the lookup-history entry overhead) is counted always
but only add BigInt(preimageBlob.length) via sumU64 and increment
calculatedStorageCount with sumU32 when a preimage is actually provided (i.e.,
when preimageBlob is non-null/defined and slots.length >= 1 and you push
UpdatePreimage.provide/PreimageItem.create); update the code around
calculatedStorageBytes, calculatedStorageCount, sumU64, sumU32, preimageBlob,
slots and the UpdatePreimage.provide block so blob bytes and the extra count are
only added inside the branch that creates the preimage.
Resolves #48
Proposed YAML schema: