fetch Chelonia option by corrideat · Pull Request #2756 · okTurtles/group-income

corrideat · 2025-04-20T19:36:05Z

This is based on PR 'Offline cache' #2508, addressing issue #2503. The reason for this separate PR is that it might take a while to have #2508 ready, but these changes are generally useful.

These changes add a fetch option to Chelonia to override the global fetch. This is useful in many contexts, such as for debugging, for adding custom logic and other things. In PR #2508, this option is used for caching some API calls (since the network activity happens inside of the SW, the fetch event isn't used)

corrideat · 2025-04-20T19:38:10Z

shared/domains/chelonia/internals.js

-    if (sourceHash !== contractInfo.hash) {
-      throw new Error(`bad hash ${sourceHash} for contract '${contractInfo.file}'! Should be: ${contractInfo.hash}`)
-    }
+    const source = await sbp('chelonia/out/entry', contractInfo.hash, { code: multicodes.SHELTER_CONTRACT_TEXT })


This new addition also DRY'es some of the code.

cypress · 2025-04-20T19:49:54Z

group-income Run #4410

Run Properties: Passed #4410 • 4ae569d043 ℹ️: Merge c108e41d71ee8e0786ce6e4fcc4e6519d38ea861 into 51937576a77a67cf30711cad5006...

Project	`group-income`
Branch Review	`fetch-option`
Run status	`Passed #4410`
Run duration	`15m 46s`
Commit	`4ae569d043 ℹ️: Merge c108e41d71ee8e0786ce6e4fcc4e6519d38ea861 into 51937576a77a67cf30711cad5006...`
Committer	`Ricardo Iván Vieitez Parra`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`10`
Skipped	`0`
Passing	`116`
View all changes introduced in this branch ↗︎

taoeffect · 2025-04-21T17:26:13Z

shared/domains/chelonia/chelonia.js

      return state
    })
  },
+  'chelonia/out/entry': async function (cid: string, { code }: { code?: number } = {}) {


This is a neat new addition, but could you come up with a better name for it?

Right now it reads like it's sending an SPMessage, and that's not what it's doing.

chelonia/out/fetchEntry?

The selector should be descriptive of what the code is doing — without confusing the reader. The word entry is verboten here as it refers to SPMessage in many other places, and this has nothing to do with SPMessage.

So something like chelonia/getFile or chelonia/fetchVerifiedFile.

I don't like out because the meaning of chelonia/out is sending something to the server, which this code isn't doing.

I think it make sense to always verify the CID, as that's what the code did before, so chelonia/fetchVerifiedFile or chelonia/fetchFile would make sense, especially with the other comment here:

#2756 (comment)

Fine about entry. I think file is also confusing for similar reasons, but we can go with that, or verifiedFile.

Re. /out/, it is sending something to the server (just not storing something on the sever). We already have 'chelonia/out/latestHEADInfo'.

Re. /out/, it is sending something to the server (just not storing something on the sever). We already have 'chelonia/out/latestHEADInfo'.

That's a good point...

We should also consider then the existing selector 'chelonia/fileDownload'. Having this selector be named chelonia/fetchVerifiedFile would be confusing as it would somewhat conflict with that selector.

Let me know if you have any additional ideas based on this...

taoeffect · 2025-04-21T17:41:13Z

shared/domains/chelonia/chelonia.js

+    const local = await sbp('chelonia.db/get', cid)
+    if (local) return local


o4-mini-high:

1) chelonia/out/entry’s “if (local)” drops empty‐string or zero‐length values

// shared/domains/chelonia/chelonia.js + 'chelonia/out/entry': async function (cid: string, { code }: { code?: number } = {}) { + const parsedCID = parseCID(cid) + … + const local = await sbp('chelonia.db/get', cid) + if (local) return local + … + }

Problem: if your DB ever stores an empty string ("") or some other legitimately falsy value, you’ll always fall back to the network rather than returning your cache.

Suggested fix: check explicitly for null/undefined, not truthiness:

'chelonia/out/entry': async function (cid: string, { code }: { code?: number } = {}) { // first check the cache const local = await sbp('chelonia.db/get', cid) if (local != null) { return local } // now validate the CID const parsedCID = parseCID(cid) if (Number.isInteger(code) && parsedCID.code !== code) { throw new Error('Invalid CID content type') } const url = `${this.config.connectionURL}/file/${cid}` const data = await this.config.fetch(url, { signal: this.abortController.signal }) .then(handleFetchResult('text')) // verify integrity const ourHash = createCID(data, parsedCID.code) if (ourHash !== cid) { throw new Error(`expected hash ${cid}. Got: ${ourHash}`) } await sbp('chelonia.db/set', cid, data) return data }

My thinking has evolved a bit here, now I'm for removing these lines completely: #2756 (comment)

taoeffect · 2025-04-21T17:48:58Z

shared/domains/chelonia/internals.js

        // See: https://github.com/okTurtles/group-income/issues/531
        try {
-          const response = await fetch(`${this.config.connectionURL}/time`, { signal: this.abortController.signal })
+          const response = await this.config.fetch(`${this.config.connectionURL}/time`, { signal: this.abortController.signal })


Pretty sure o4-mini-high is wrong about this, right?

4) Embedded this.config.fetch inside new Function / sesImportVM

// shared/domains/chelonia/internals.js - const response = await fetch(`${this.config.connectionURL}/time`, { … }) + const response = await this.config.fetch(`${this.config.connectionURL}/time`, { … })

This snippet lives inside a template‐string passed to new Function. At runtime the this in that sandboxed code almost certainly will not point back to your SBP domain, so this.config will be undefined.

You’ll either need to:

Inject your fetch into the sandbox closure explicitly (e.g. add a const fetch = parentFetch at the top), or

Bind the generated function to the correct this at invocation time, e.g.
const fnFactory = new Function(codeAsString) const sandboxFactory = fnFactory.call(this) // `this` is your domain
so that inside fetchServerTime, this.config is available.

Without that you’ll get TypeError: Cannot read property 'config' of undefined.

I think this might be right, but we probably shouldn't spend too much time on this since this code is commented out anyhow. When and if this comes into use, we need to ensure that references work properly, and IMHO it's better to have this.config.fetch and have that fail (so that we can correct it) than have fetch and have it potentially work.

taoeffect · 2025-04-21T17:54:54Z

shared/domains/chelonia/chelonia.js

+    if (Number.isInteger(code)) {
+      if (parsedCID.code !== code) {
+        throw new Error('Invalid CID content type')
+      }
+    }


Should we throw if code isn't an integer?

Also, Gemini 2.5 Pro Preview has a decent suggestion to improve the error here, although I don't think the second error for the hash needs to be modified:

chelonia/out/entry Error Specificity

In chelonia/out/entry, the error thrown when the content type code doesn't match is a generic Error. It would be slightly more informative to include the expected and actual codes. Similarly, the hash mismatch error could be more specific or use a custom error type if data integrity failures need special handling elsewhere.

--- a/shared/domains/chelonia/chelonia.js +++ b/shared/domains/chelonia/chelonia.js @@ -1053,12 +1056,12 @@ export default (sbp('sbp/selectors/register', { const parsedCID = parseCID(cid) if (Number.isInteger(code)) { if (parsedCID.code !== code) { - throw new Error('Invalid CID content type') + throw new Error(`Invalid CID content type. Expected ${code}, got ${parsedCID.code}`) } } const local = await sbp('chelonia.db/get', cid) if (local) return local const url = `${this.config.connectionURL}/file/${cid}` const data = await this.config.fetch(url, { signal: this.abortController.signal }).then(handleFetchResult('text')) const ourHash = createCID(data, parsedCID.code) if (ourHash !== cid) { - throw new Error(`expected hash ${cid}. Got: ${ourHash}`) + throw new Error(`CID hash mismatch for ${cid}. Calculated hash: ${ourHash}`) // Or potentially: new ChelErrorDataIntegrity(...) } await sbp('chelonia.db/set', cid, data) return data

if (Number.isInteger(code)) { is meant to allow passing undefined or null to skip this check. Maybe I could check for that instead?

shared/domains/chelonia/time-sync.js

taoeffect · 2025-04-21T18:01:18Z

shared/domains/chelonia/chelonia.js

+    if (ourHash !== cid) {
+      throw new Error(`expected hash ${cid}. Got: ${ourHash}`)
+    }
+    await sbp('chelonia.db/set', cid, data)


The use of a cache is a new addition... I suppose it is safe to use because it is based on cids. Just making a note though as this is new logic being added that wasn't there before, not just DRYing.

Good point. Well, this does come from the offline cache PR so that's why. I didn't notice it as I made this PR it added new functionality.

Per the call: in 99% of cases the lightweight client is true and this therefore does nothing.

Therefore at minimum this needs to be commented here, but also IMO it would be better to not do this type of caching here anyway and rely only on the browser cache (whether it works or not), because then you'd be able to compare the two values and not have to worry about the comment above regarding if (local) ... i.e. you'd always verify that the CID corresponds to "" and therefore would have to have an if (local != null) check or any corresponding cache code here. IMO that would simplify this code and so we should do it. I don't think it would affect performance much.

In the offline cache PR I think you said also do the caching setup in either the fetch implementation or chelonia.db/get anyway, so therefore this caching code would be possibly redundant?

I'm not sure this really is redundant, as 'chelonia/private/db/addEntry' has await sbp('chelonia.db/set', entry.hash(), entry.serialize()), which similarly does nothing for the lightweight client. Adding contract manifests and source to the DB when events are added seemed consistent, since events can't be processed without the corresponding contract.

Regardless of everything else, I think we need to code needs to get rid of these lines:

const local = await sbp('chelonia.db/get', cid) if (local) return local

The previous code didn't have this, and the previous code also always verified that the CID matched — we need to restore that functionality (of verifying the CID always matches).

But the current logic does verify the CID matches, since they had to to get added to the DB.

Ah... good point. OK feel free to disregard that, and/or perhaps add a comment to that effect.

taoeffect

Great work @corrideat! Left some feedback!

taoeffect

Nice work @corrideat !

fetch Chelonia option

392fecf

corrideat requested a review from taoeffect April 20, 2025 19:36

corrideat self-assigned this Apr 20, 2025

corrideat commented Apr 20, 2025

View reviewed changes

Use this.config.fetch in some locations

d8fa633

corrideat added this to the Chelonia changes milestone Apr 20, 2025

taoeffect reviewed Apr 21, 2025

View reviewed changes

shared/domains/chelonia/time-sync.js Show resolved Hide resolved

taoeffect reviewed Apr 21, 2025

View reviewed changes

taoeffect requested changes Apr 21, 2025

View reviewed changes

Feedback

043b177

corrideat requested a review from taoeffect April 22, 2025 14:49

Empty

c108e41

taoeffect approved these changes Apr 22, 2025

View reviewed changes

taoeffect merged commit 2068d4f into master Apr 22, 2025
4 checks passed

taoeffect deleted the fetch-option branch April 22, 2025 17:08

		const local = await sbp('chelonia.db/get', cid)
		if (local) return local

Uh oh!

Conversation

corrideat commented Apr 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cypress bot commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

1) chelonia/out/entry’s “if (local)” drops empty‐string or zero‐length values

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

4) Embedded this.config.fetch inside new Function / sesImportVM

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taoeffect left a comment

Choose a reason for hiding this comment

Uh oh!

taoeffect left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cypress bot commented Apr 20, 2025 •

edited

Loading

4) Embedded `this.config.fetch` inside `new Function` / `sesImportVM`