Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32e440a944
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (best && candidate.checkpoint <= best.checkpoint) { | ||
| continue |
There was a problem hiding this comment.
With allUpdates: true, candidates are sorted by ascending checkpoint, but this condition still allows every later valid candidate to replace best, so intermediate updates are skipped. If two valid updates exist from the same starting config (e.g., checkpoints 10 and 20), the method returns only 20 instead of the full path, which violates the intent of returning all updates. The allUpdates mode should stop on the first valid candidate (smallest checkpoint) before recursing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd5c86f047
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const payloadPromise = payloads.get(subdigest) ?? this.getPayload(subdigest) | ||
| payloads.set(subdigest, payloadPromise) | ||
| const payload = await payloadPromise | ||
|
|
There was a problem hiding this comment.
This path trusts the signature record’s Subdigest and immediately uses the fetched payload without validating that it belongs to the same wallet (or that the subdigest is actually hash(wallet, chainId, payload)). A malformed or spoofed Arweave signature item can therefore make getWallets (and the similar getWitnessGeneric flow) return a witness for wallet A that actually points to wallet B’s payload, producing incorrect wallet associations and invalid update/signature chains downstream.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c25b88ec6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const candidate = candidates.get(nextImageHash) ?? { | ||
| nextImageHash, | ||
| checkpoint, | ||
| signatureEntries: new Map<string, ItemEntry>(), | ||
| } |
There was a problem hiding this comment.
getConfigurationUpdates buckets candidate signatures only by To-Config, then aggregates one signature per signer across that bucket. This lets signatures from different subdigests (for example, the same target image hash signed under different chain IDs or payload contexts) be combined to satisfy threshold even though no single update payload has quorum. In that case the reader can return an update step that is not actually executable/valid for any real subdigest, corrupting the computed update path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7690d76c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const entry of await this.findEntries({ Type: 'config update', Wallet: normalizedWallet })) { | ||
| const { tags } = entry | ||
| const nextImageHash = normalizeHex(tags['To-Config'] as Hex.Hex) | ||
| const checkpoint = BigInt(tags['To-Checkpoint'] as string) |
There was a problem hiding this comment.
getConfigurationUpdates ranks and filters candidates using the unverified 'To-Checkpoint' tag from each signature record instead of the checkpoint from the actual target configuration. If that tag is stale or malformed, the method can pick the wrong hop (or recurse incorrectly) even when the underlying config data disagrees. This reader already has getConfiguration(nextImageHash) available, so candidate ordering/validation should use the resolved config checkpoint to avoid returning an invalid update path.
Useful? React with 👍 / 👎.
This reverts commit b7690d7.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d519dba91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let configuration: Config.Config | undefined | ||
|
|
||
| for (const record of await Promise.all(configEntries.map((entry) => this.loadRecord<ArweaveConfigRecord>(entry)))) { | ||
| configuration = mergeConfigurations(configuration, fromConfigCarrier(record)) |
There was a problem hiding this comment.
getConfiguration merges records solely based on Arweave tags and never checks that the decoded configuration actually hashes to the imageHash being requested. If a malformed or mis-tagged record is the only match, this can return an unrelated configuration and corrupt signer/update resolution for that wallet. Recompute Config.hashConfiguration(...) for each loaded record and reject records whose content hash does not match the requested hash.
Useful? React with 👍 / 👎.
| return { | ||
| chainId: Number(record['Chain-ID']), | ||
| payload: fromPayloadRecord(record), | ||
| wallet: normalizeAddress(record.Address), | ||
| } |
There was a problem hiding this comment.
getPayload trusts the Payload tag and returns the decoded body without recomputing the digest from (wallet, chainId, payload). When a record has an incorrect/stale tag, callers can receive a payload for a different subdigest, and downstream flows that consume this helper can associate signatures with the wrong operation. Compute the expected digest and skip or reject mismatched records.
Useful? React with 👍 / 👎.
This reverts commit 3d519db.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57769889a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ('Context-Factory' in record) { | ||
| return { | ||
| factory: normalizeAddress(record['Context-Factory']), | ||
| stage1: normalizeAddress(record['Context-Stage-1']), | ||
| stage2: normalizeAddress(record['Context-Stage-2']), | ||
| creationCode: normalizeHex(record['Context-Creation-Code']), | ||
| } |
There was a problem hiding this comment.
When custom context tags are present, inferContext returns them directly without checking that SequenceAddress.from(Deploy-Config, context) equals record.Wallet. A malformed wallet record can therefore make getDeploy(wallet) return an image hash/context pair that does not actually derive that wallet, which breaks counterfactual-address assumptions and can cause callers to act on the wrong deployment metadata.
Useful? React with 👍 / 👎.
| Tree: normalizeHex(imageHash), | ||
| }) | ||
|
|
||
| return record ? fromTreeData(record.data) : undefined |
There was a problem hiding this comment.
getTree trusts the Tree tag and returns decoded content without recomputing its root hash. If a record is mis-tagged or corrupted, this returns a tree for a different root while callers believe it matches the requested imageHash, which can corrupt downstream tree/proof resolution.
Useful? React with 👍 / 👎.
| const topology = toRecoveredLikeTopology(minimalTopology) | ||
| const { weight } = Config.getWeight(topology, () => false) | ||
| if (weight < currentConfig.threshold) { |
There was a problem hiding this comment.
This quorum check counts leaves as signed based on signature presence, but it never verifies that those signatures are valid for the config-update subdigest. Because toRecoveredLikeTopology marks provided leaves as signed and Config.getWeight then credits full weight, malformed or forged signature records can be accepted as a valid update path.
Useful? React with 👍 / 👎.
1 participant
No description provided.