feat: dynamically discover storage keys from transaction footprints - #271
Conversation
|
@Glam26 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesFootprint-based storage key discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/core/discovery.test.ts`:
- Around line 29-31: The discovery test fixture in the tx-hash deduplication
scenario is not actually creating a duplicate, so the test does not validate the
intended behavior. Update the mock data in discovery.test.ts to reuse the same
txHash for the duplicate entry, then adjust the expected scan and key counts in
the associated assertions so they reflect one deduplicated hash. Use the
existing test case and helpers around the discovery scan flow to keep the change
localized.
- Around line 97-101: The `TransactionExt.1` construction in the
`tests/core/discovery.test.ts` fixture is invalid TypeScript and prevents the
test file from parsing. Remove the broken
`actualModule.xdr.TransactionExt.1(...)` usage in the affected test setup, and
either replace it with a valid Soroban transaction fixture or delete the unused
`txExt` block entirely; use the surrounding
`actualModule.xdr.TransactionV1EnvelopeExt` and `sorobanData` test setup to
locate the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ca7885f-0ee8-4346-b42f-b0d72ac37a6f
📒 Files selected for processing (2)
src/core/discovery.tstests/core/discovery.test.ts
📜 Review details
🧰 Additional context used
🪛 Biome (2.5.0)
tests/core/discovery.test.ts
[error] 97-97: Expected a semicolon or an implicit semicolon after a statement, but found none
(parse)
🔇 Additional comments (1)
src/core/discovery.ts (1)
122-205: LGTM!Also applies to: 211-213
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
Hi @Glam26, the CI is failing with a syntax error in \ ests/core/discovery.test.ts\ due to this line: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/core/discovery.test.ts (1)
96-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove stale scratch comment.
This "wait, xdr.TransactionExt is a union?..." note is leftover thinking-out-loud text; the code below it already resolved to the simpler
mockEnvelopeobject. Clean it up for clarity.Proposed fix
- // Build TransactionEnvelope - // wait, xdr.TransactionExt is a union? Yes. It might be simpler to just return a mock object that matches the structure. - + // Build a mock TransactionEnvelope-like object exposing the accessor methods + // the discovery code calls (switch/v1/tx/ext) instead of real XDR serialization. const mockEnvelope = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/discovery.test.ts` around lines 96 - 98, Remove the leftover scratch note in the TransactionEnvelope setup within the discovery test; the comment about xdr.TransactionExt being a union is stale and the code already uses the simpler mockEnvelope approach. Clean up the nearby test setup in discovery.test.ts so only the intended explanatory comment remains, keeping the mockEnvelope construction clear and readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/core/discovery.test.ts`:
- Around line 96-98: Remove the leftover scratch note in the TransactionEnvelope
setup within the discovery test; the comment about xdr.TransactionExt being a
union is stale and the code already uses the simpler mockEnvelope approach.
Clean up the nearby test setup in discovery.test.ts so only the intended
explanatory comment remains, keeping the mockEnvelope construction clear and
readable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c1b4172d-e5a9-4006-af4e-51d63c7963f3
📒 Files selected for processing (3)
src/core/discovery.tstests/core/discovery.test.tstests/rpc/client.test.ts
💤 Files with no reviewable changes (1)
- src/core/discovery.ts
📜 Review details
🔇 Additional comments (3)
tests/core/discovery.test.ts (2)
27-34: 🎯 Functional CorrectnessMock still doesn't exercise tx-hash deduplication (previously flagged).
hash1,hash2,hash3are three distinct strings, so the "// Duplicate" label onhash2is misleading and no deduplication is actually tested. All downstream assertions (transactionsScanned,newKeysDiscovered,upsertEntrycall count at Lines 183-185, 190, 200) are built around this un-deduplicated 3-transaction count, so the PR's stated dedup requirement remains unverified.Proposed fix
return { events: [ { txHash: "hash1" }, - { txHash: "hash2" }, // Duplicate + { txHash: "hash1" }, // Duplicate { txHash: "hash3" } // New ] };And update the expected counts at Lines 183-185, 190, and 200 to reflect 2 unique transactions / 4 keys instead of 3/6.
Also applies to: 174-186, 188-191, 200-203
4-4: LGTM!Also applies to: 23-26, 36-95, 99-127
tests/rpc/client.test.ts (1)
116-130: LGTM!
3 participants
I've completed the implementation for dynamic discovery of persistent
storage keys. Here is a summary of the work done:
• Re-wrote tests/core/discovery.test.ts to mock out getTransaction
and model the extraction of the transaction footprint ( envelopeXdr ),
ensuring both tests for successful parses and edge cases are accounted
for.
• Tested that keys for foreign contracts are ignored and keys for the
watched contract are automatically tracked and deduplicated.
• Modified discoverStorageKeys to extract deduplicated txHash es
from getEvents results.
• For each transaction, queried getTransaction and parsed the
transaction envelope using the Stellar SDK ( xdr.TransactionEnvelope.
fromXDR() ).
• Extracted both readOnly and readWrite ledger keys from tx.ext().
v1().sorobanData().resources().footprint() .
• Addressed network data formats by ensuring raw buffer comparison (
Buffer.compare ) when matching the contractId against the footprint's
target contract, preventing issues with format mismatches (like Buffer
vs Uint8Array ).
• All newly identified keys that match the contract ID are validated
against the contract_entries sqlite table.
• Automatically distinguished whether the entries are temporary ,
persistent , or instance types using their xdr.
ContractDataDurability .
closes #151