Skip to content

contracts/stream_contract: update stream storage tests only#117

Merged
ogazboiz merged 17 commits intoLabsCrypt:mainfrom
Obiajulu-gif:stream_storage
Feb 25, 2026
Merged

contracts/stream_contract: update stream storage tests only#117
ogazboiz merged 17 commits intoLabsCrypt:mainfrom
Obiajulu-gif:stream_storage

Conversation

@Obiajulu-gif
Copy link
Copy Markdown
Contributor

This pull request introduces new types for stream data modeling and adds comprehensive tests for serialization and authorization logic in the stream contract. The main focus is on ensuring deterministic serialization of contract types and verifying proper authorization for withdrawals.

Stream Data Modeling Enhancements:

  • Added DataKey enum and Stream struct with the #[contracttype] attribute for deterministic serialization and contract storage. (contracts/stream_contract/src/lib.rs)

Authorization and Serialization Tests:

  • Added tests to verify:

    • Only authorized recipients can withdraw from a stream, and unauthorized attempts fail. (contracts/stream_contract/src/test.rs)
    • The Stream struct serializes and deserializes as expected, matching the expected XDR layout. (contracts/stream_contract/src/test.rs)
    • The DataKey::Stream variant serializes deterministically, deserializes correctly, and works as a storage key in contract state. (contracts/stream_contract/src/test.rs)

    Closes Contract: Stream Storage Structure #77

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. This has conflicts with main. Also, the Stream struct diverges from the one on main. Please rebase and align with the existing architecture.

@Obiajulu-gif
Copy link
Copy Markdown
Contributor Author

@ogazboiz i have fix it

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. The title says 'Stream storage' but the only change in the PR is adding 'extern crate std;' to the test file. If you intended to introduce storage changes, they seem to be missing from this branch. Please check and re-push! 🙏

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on my earlier review: the latest main has significant backend and contract changes merged. The only diff in this PR is 2 lines in test.rs. Please clarify: is this PR about actual stream storage (persistent state in the Soroban contract using ledger storage), or just the test changes? If there's real storage logic, please rebase on latest main and add the implementation — the title and description don't match the diff as it stands.

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on my earlier review: the latest main has significant backend and contract changes merged. The current diff only shows 2 lines in test.rs. Please clarify if this PR is meant to include actual stream storage logic as the title suggests. If so, please rebase and add the implementation. If not, please update the title/description.

@Obiajulu-gif
Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up. You're right: after rebasing against the latest main, the stream-storage implementation is already present from the earlier branch, and this PR now only contains small test.rs updates. I'll update the title/description to reflect the test-only scope (or close this PR if preferred).
@ogazboiz

@Obiajulu-gif Obiajulu-gif changed the title Stream storage contracts/stream_contract: update stream storage tests only Feb 23, 2026
Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @Obiajulu-gif, glad to hear you fixed it! the PR is still showing as conflicting though — could you just resolve the merge conflict and push? run git fetch origin && git rebase origin/main then force push. once that's done we can take another look and get it merged!

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey Obiajulu — thanks for rebasing!

i noticed the CI checks are now failing on this branch: the "Dependency Vulnerability Scan" is failing.

could you take a look at the workflow logs and see what's throwing the error? you can check the details under the "Checks" tab on GitHub. let me know if you need any help deciphering the CI logs!

@Obiajulu-gif
Copy link
Copy Markdown
Contributor Author

@ogazboiz I have fix the issues

@ogazboiz
Copy link
Copy Markdown
Contributor

@ogazboiz I have fix the issues

still failing

@ogazboiz
Copy link
Copy Markdown
Contributor

@Obiajulu-gif just one check for it to pass
image

@Obiajulu-gif
Copy link
Copy Markdown
Contributor Author

@ogazboiz I have fix the issues

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, merging this in! thanks for the contribution

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey! unfortunately, another PR was just merged that caused merge conflicts with this one. could you run git fetch origin && git rebase origin/main to resolve them? once they are fixed and CI passes again, i'll merge this right in!

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey! great news — the CI is passing on this branch now!

however, it looks like some new changes on main have caused a merge conflict. could you please rebase one last time to resolve those?

once the conflict is cleared and CI passes again, i'll merge this right in. almost there!

@ogazboiz
Copy link
Copy Markdown
Contributor

i have resolve it for you @Obiajulu-gif

@ogazboiz ogazboiz merged commit 1a9b15f into LabsCrypt:main Feb 25, 2026
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Contract: Stream Storage Structure

2 participants