initial files

[corrideat]

No description provided.

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@​chelonia/​serdes@​1.0.0
	@​apeleghq/​rfc8188@​1.0.8
	@​sbp/​okturtles.data@​0.1.6
	@​sbp/​okturtles.events@​1.0.1
	@​apeleghq/​multipart-parser@​1.0.18
	@​sbp/​okturtles.eventqueue@​1.2.1
	@​sbp/​sbp@​2.4.1
	turtledash@​1.0.2
	@​typescript-eslint/​eslint-plugin@​8.33.0
	ts-node@​10.9.2
	@​types/​node@​22.15.29
	eslint-config-standard@​17.1.0
	typescript@​5.8.3
	@​chelonia/​crypto@​1.0.1

View full report

[taoeffect]

I think, before merging this, we should pass every single file through an LLM comparing it to the original file and ask the LLM if there are any differences between the files unrelated to type signature changes.

[taoeffect]

I ran this amazing script that @corrideat wrote:

Details

#!/bin/sh

git clone https://github.com/okTurtles/group-income.git
git clone https://github.com/okTurtles/libcheloniajs.git
( cd libcheloniajs; git checkout initial )

cwd=$CWD

(
    cp -Ra group-income/shared/domains/chelonia/ gi && \
    cp group-income/shared/functions.js group-income/shared/pubsub.js gi && \
    rm -rf gi/*.test.js && \
    rm -rf group-income
) || exit 1

(
    cp -Ra libcheloniajs/src/ lc && \
    mv lc/local-selectors/index.ts lc/localSelectors.ts && \
    mv lc/pubsub/index.ts lc/pubsub.ts && \
    rm -rf lc/local-selectors lc/pubsub lc/*.test.ts lc/index.ts lc/types.ts && \
    rm -rf libcheloniajs
) || exit 1

X=$(cd gi; find . -type f|sort) Y=$(cd lc; find . -type f|sort|sed -e 's/.ts$/.js/g'); [ x"$X" = x"$Y" ] || ( echo "File list does not match" >&2; exit 1)

mkdir -p 'dist'

yes '' | npm init

npm install --save-dev 'esbuild' 'eslint@8' 'eslint-config-standard' 'flow-remove-types'
cat >index.mjs <<-EOF
import * as esbuild from 'esbuild'
import { loadESLint } from 'eslint'
import flowRemoveTypes from 'flow-remove-types'

const EslintConstructor = await loadESLint()
const eslint = new EslintConstructor({ fix: true })

const extractSource = async (source, isFlow) => {
  const elideTypes = isFlow ? flowRemoveTypes : (v) => v
  return (await eslint.lintText(esbuild.transformSync(elideTypes(source, { all: true }).toString(), { loader: 'ts' }).code
  ))[0].output
}

(() => {
  const data = []
  process.stdin.on('readable', () => {
    let chunk

    while ((chunk = process.stdin.read()) !== null) {
      data.push(chunk)
    }
  })

  process.stdin.on('end', () => {
    extractSource(Buffer.concat(data).toString(), process.argv[2] === 'flow').then((v) => {
      v && process.stdout.write(v)
    })
  })
})()
EOF

cat >.eslintrc.json <<-EOF
{"extends":"standard"}
EOF

for i in gi lc; do
    flow=''
    [ x"$i" = x'gi' ] && flow='flow'
    find $i -type f '(' -name '*.js' -o -name '*.ts' ')' | while read line; do
        mkdir -p 'dist/'"$(dirname "$line")"
        node index.mjs "$flow" < "$line" > 'dist/'"${line%.*}.js"
    done
done

(
    ( cd gi && find . -type f ) | while read i; do diff -up "dist/gi/$i" "dist/lc/$i"; done
)

And then ran the diff through a few different LLMs with this prompt:

Please review the following diff and let me know if you see any changes that affect any logic:

Here are the things it noticed that I think need to be addressed or might need to be addressed:

o4-mini

3. Function Parameter Changes:

   • In chelonia.js, the function signature for eventsBetween has changed from:

     'chelonia/out/eventsBetween': function (contractID, startHash, endHeight, offset = 0, { stream } = { stream: true }) {

     to:

     'chelonia/out/eventsBetween': function (contractID, { startHash, endHeight, offset = 0, limit = 0, stream = true }) {

     This change encapsulates startHash, endHeight, offset, limit, and stream into a single object. This could impact how this function is called elsewhere in the code, requiring updates to the function calls to match the new parameter structure.

4. Logic Changes in Event Handling:

   • The logic for calculating the limit has been changed:

     const ourLimit = limit ? Math.min(endHeight - startOffset + 1, limit) : endHeight - startOffset + 1

     This could affect how many events are returned by the eventsBetween function, particularly if limit is not passed or is zero. Ensure that this logic aligns with the intended behavior.

5. Error Handling:

   • The addition of handleFetchResult in functions.js introduces a new utility function for handling fetch results, which could centralize error handling for network requests. This is a positive change, but make sure that all places where fetch results are handled are updated to use this new function.

Opus 4

1. chelonia/out/eventsBetween signature change (Critical)

-  'chelonia/out/eventsBetween': function (contractID, startHash, endHeight, offset = 0, { stream } = { stream: true }) {
+  'chelonia/out/eventsBetween': function (contractID, { startHash, endHeight, offset = 0, limit = 0, stream = true }) {

This is a breaking API change. The function now expects an object as the second parameter instead of individual parameters. Any code calling this function with the old signature will break.

2. New limit parameter logic in eventsBetween

-        const limit = endHeight - startOffset + 1
-        if (limit < 1) {
+        const ourLimit = limit ? Math.min(endHeight - startOffset + 1, limit) : endHeight - startOffset + 1
+        if (ourLimit < 1) {

The function now supports a limit parameter that can cap the number of events returned. When limit is provided and non-zero, it will return at most limit events instead of all events in the range.

7. Removed 'use strict' directives

Multiple files had 'use strict' removed, suggesting the code is now ES modules by default.

---

Note: I'm not sure about 'use script'. Also not sure handleFetchResult. Those might be fine, but please double-check.

Also found an issue in ci.yml (see comment below).

And finally, please make sure that if there are any types that need to be updated (e.g. for chelonia/out/eventsBetween signature), that they're updated too because I don't think those are included in the diff.

[corrideat]

Regarding the rest of the feedback:

The changes to eventsBetween are related to fetching chatroom messages in both directions. This is part of the branch I'm currently working on in okTurtles/group-income#2841. This change needs to be ported to PR okTurtles/group-income#2825 once this is merged.

The use strict directive isn't needed because it's automatically added by TypeScript where appropriate (note the files under dist/cjs have it; it's not needed for ESM because ESM is exclusively strict mode).

The handleFetchResult function was imported from GI. Since this is a separate library, handleFetchResult needed to be copied over (however, we could avoid duplicating code by importing it from GI).

[taoeffect]

In your other PR this line has been changed to:

reader = sbp('chelonia/out/eventsAfter', contractID, startOffset, ourLimit).getReader()

Did you forget to update it here?

[corrideat]

It looks like I forgot. Good catch.

[taoeffect]

These two APIs are now inconsistent with the API of eventsBetween.

When I made my comment here, I thought you would only move the optional parameters into the object, but you moved almost all of them.

I have no problem with that, I actually prefer that all parameters be named, but then these two APIs, 'chelonia/out/eventsAfter', and 'chelonia/out/eventsBefore', need to be updated to be consistent with how 'chelonia/out/eventsBetween' is structured too.

In otherwords, everything after contractID needs to be moved into an object.

[corrideat]

This would be a more involved change, as those functions are used in other places besides ChatMain.vue (in particular, eventsAfter is used in Chelonia itself). Should I go ahead with making the change to named parameters, or should I partially revert the changes to eventsBetween (and if the latter, please provide the desired signature)?

[taoeffect]

The APIs should be consistent... so I don't know what you mean by "revert". The problem with eventsBetween before was a real problem. One solution to it would be to move those optional params into an options object like eventsBefore has. Another solution (that you picked) would be to move all of them (except contractID) into an object.

Reverting to how it was before isn't an option because it was really poorly designed before... So if by "revert" you mean "go with the solution where optional params are placed in an options object", that's fine too. But if by "revert" you mean, "undo our improvement"... I don't think that's a good idea.

[corrideat]

I'm agreeing with that they should ideally be consistent. OTOH, it looks like I was overzealous with the way I implemented named parameters.

So, there are two ways I see to address the inconsistency issue:

1. Make the same change to eventsAfter and eventsBefore, i.e., making everything after contractID a named parameter.
2. Partially revert the change to eventsBetween. This would in practice mean keeping some arguments positional (as they were) and some named (other than stream)

Both alternatives are pretty simple, but the first option (making everything named) will require more care to ensure no mistakes are introduced (e.g., by forgetting to update a call to the new signature).

[corrideat]

Another question (I guess it applies to either option, since both are breaking changes):

Would you rather I update the code here (in this repo) and in okTurtles/group-income#2825, or that I make a separate PR to https://github.com/okTurtles/group-income with the same API changes (this way, okTurtles/group-income#2825 will continue to have just import-related changes)?

[taoeffect]

@corrideat

Would you rather I update the code here (in this repo) and in okTurtles/group-income#2825, or that I make a separate PR to https://github.com/okTurtles/group-income with the same API changes (this way, okTurtles/group-income#2825 will continue to have just import-related changes)?

It sounds to me like the latter aligns closer with being script-friendly. I think as a matter of principle and due-diligence on both of our parts to avoid headaches for both of us and our users we should only merge this PR (and PR okTurtles/group-income#2825), after all Chelonia changes have been merged into master in Group Income, so that we can run a final diff on the files and be 100% sure we haven't accidentally missed something.

[corrideat]

okTurtles/group-income#2850

[taoeffect]

Congrats @corrideat!!