feat(tokenomics): transfer LP position NFT to liquidity owner#48
feat(tokenomics): transfer LP position NFT to liquidity owner#48
Conversation
The StreamComponent creates an Ekubo LP position during provide_initial_liquidity() but permanently locks the position NFT inside the stream token contract. This adds a liquidity_owner field to LiquidityConfig and transfers the LP position NFT to that address immediately after minting, allowing the owner to manage or withdraw liquidity. - Add liquidity_owner to LiquidityConfig struct and IStreamToken trait - Transfer LP NFT via IERC721 after position creation - Emit LiquidityPositionTransferred event - Validate liquidity_owner is non-zero in initializer - Update deploy script, example configs, and all test sites Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Summary of ChangesHello @loothero, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the tokenomics system by introducing explicit ownership for Liquidity Provider (LP) position NFTs. Previously, these NFTs remained within the stream token contract, limiting their utility. The changes now enable these NFTs to be transferred to a specified Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @loothero's task —— View job Claude Review - General Engineering Reviewrun=22183928383 attempt=1 sha=1c4ce167ce9dab818a8bb4294fee4214e8570a25 scope=general Todo List
FindingsNo issues found. AnalysisThe changes outside Script Changes (
JSON Example Files
Security Considerations
Maintainability
CI/CD
Summary0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
|
Claude finished @loothero's task —— View job Claude Review - Cairo/Starknet Contract ReviewTodo List
run=22183928383 attempt=1 sha=1c4ce167ce9dab818a8bb4294fee4214e8570a25 scope=packages Summary[HIGH] packages/economy/src/tokenomics/stream/stream.cairo:313 - Missing return value check on ERC721 transfer let transfer_success = nft.transfer_from(get_contract_address(), liquidity_owner, position_id.into());
assert(transfer_success, 'NFT transfer failed');[HIGH] packages/economy/src/tokenomics/stream/stream.cairo:310-313 - Missing interface validation for external NFT contract let nft_address = positions_dispatcher.get_nft_address();
let nft = IERC721Dispatcher { contract_address: nft_address };
// Validate ERC721 interface support via SRC5
assert(nft.supports_interface(IERC721_ID), 'Invalid NFT contract');[MEDIUM] packages/economy/src/tokenomics/stream/stream.cairo:309-314 - Potential reentrancy risk via external NFT call // Emit event first
self.emit(LiquidityPositionTransferred { position_id, owner: liquidity_owner });
// Then perform external call
nft.transfer_from(get_contract_address(), liquidity_owner, position_id.into());[LOW] packages/economy/src/tokenomics/tests/test_stream_lifecycle.cairo:115-116 - Redundant mock calls for same interface // Keep only the correct method name based on actual contract
start_mock_call(MOCK_NFT(), selector!("transfer_from"), ());[INFO] Multiple files - Consistent test coverage updates Positive Observations
Recommendations
The core functionality is sound and well-tested, but the missing external call safety checks present security risks that should be addressed before merging. Summary: 0 CRITICAL, 2 HIGH, 1 MEDIUM, 1 LOW, 1 INFO |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a liquidity_owner field, allowing the LP position NFT to be transferred to a designated address. This is a valuable feature that enhances the flexibility of managing liquidity positions. The implementation is thorough, with changes correctly propagated across contracts, interfaces, tests, and scripts. The documentation has also been updated to reflect this new functionality. I've identified a minor area for improvement in the test mocks to enhance clarity and remove redundant code.
| fn mock_nft_transfer(mock_positions: starknet::ContractAddress) { | ||
| start_mock_call(mock_positions, selector!("get_nft_address"), MOCK_NFT()); | ||
| start_mock_call(mock_positions, selector!("transfer_from"), ()); | ||
| start_mock_call(MOCK_NFT(), selector!("transferFrom"), ()); | ||
| start_mock_call(MOCK_NFT(), selector!("transfer_from"), ()); | ||
| } | ||
|
|
||
| fn stop_mock_nft_transfer(mock_positions: starknet::ContractAddress) { | ||
| stop_mock_call(mock_positions, selector!("get_nft_address")); | ||
| stop_mock_call(mock_positions, selector!("transfer_from")); | ||
| stop_mock_call(MOCK_NFT(), selector!("transferFrom")); | ||
| stop_mock_call(MOCK_NFT(), selector!("transfer_from")); | ||
| } |
There was a problem hiding this comment.
The mock_nft_transfer and stop_mock_nft_transfer helper functions include mock calls that are not necessary for the test to pass and can be removed for clarity. The code under test only calls get_nft_address on the positions contract and transfer_from on the NFT contract. The mocks for transfer_from on the positions contract and transferFrom (with a capital F) on the NFT contract are not needed.
fn mock_nft_transfer(mock_positions: starknet::ContractAddress) {
start_mock_call(mock_positions, selector!("get_nft_address"), MOCK_NFT());
start_mock_call(MOCK_NFT(), selector!("transfer_from"), ());
}
fn stop_mock_nft_transfer(mock_positions: starknet::ContractAddress) {
stop_mock_call(mock_positions, selector!("get_nft_address"));
stop_mock_call(MOCK_NFT(), selector!("transfer_from"));
}
Codex Review - General Engineering Review[MEDIUM] scripts/examples/summit_attack_potion.json:2 - The new Summit example configs state [LOW] scripts/create_stream_token.sh:541 - Summary: 0 CRITICAL, 0 HIGH, 1 MEDIUM, 1 LOW, 0 INFO |
Codex Review - Cairo/Starknet Contract Review[MEDIUM] [LOW] Summary: 0 CRITICAL, 0 HIGH, 1 MEDIUM, 1 LOW, 0 INFO |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to transfer the Ekubo LP position NFT to a designated owner address after initial liquidity provision, instead of permanently locking it in the stream token contract. This enables the liquidity owner to manage or withdraw liquidity as needed.
Changes:
- Added
liquidity_ownerfield toLiquidityConfigstruct with validation and storage - Modified
provide_initial_liquidity()to transfer the LP position NFT to the designated owner after minting - Added
LiquidityPositionTransferredevent andget_liquidity_owner()getter function
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/interfaces/src/tokenomics/stream.cairo | Added liquidity_owner field to LiquidityConfig and get_liquidity_owner() getter to interface |
| packages/economy/src/tokenomics/stream/stream.cairo | Core implementation: storage, validation, NFT transfer logic, event, and getter |
| packages/economy/src/tokenomics/constants.cairo | Added STREAM_INVALID_LIQUIDITY_OWNER error constant |
| packages/economy/src/tokenomics/tests/*.cairo | Updated test fixtures to include liquidity_owner: OWNER() in all LiquidityConfig structs |
| packages/economy/src/tokenomics/tests/test_stream_lifecycle.cairo | Added NFT transfer mocking helpers and event assertion for 7 lifecycle tests |
| packages/presets/src/tests/test_stream_token.cairo | Updated test fixtures with liquidity_owner field |
| scripts/create_stream_token.sh | Added extraction, validation, display, and serialization of liquidity_owner |
| scripts/examples/*.json | Added liquidity_owner field to all 4 Summit example configurations |
| packages/economy/src/tokenomics/AGENTS.md | Updated documentation for LiquidityConfig struct |
Comments suppressed due to low confidence (1)
scripts/create_stream_token.sh:723
- The
CREATE_CALLDATAstring is built from unvalidated numeric config fields (e.g.,total_supply,stream_token_amount,paired_token_amount) and then interpolated unquoted into thesncastinvocation via--calldata $CREATE_CALLDATA, which allows shell metacharacters in the JSON config to break out of the argument list and execute arbitrary commands. An attacker who can supply or modify the JSON config (for example, in a CI context or when sharing example configs) can inject characters like;or command substitutions into these numeric fields, resulting in the script running unintended shell commands with the caller’s privileges. To fix this, ensure all numeric fields are strictly validated as digits-only before use and pass calldata as a properly quoted array of arguments (or otherwise avoid unquoted expansion into the shell command).
CREATE_CALLDATA="$NAME_CALLDATA $SYMBOL_CALLDATA $TOTAL_SUPPLY_WEI $PAIRED_TOKEN $POOL_FEE $STREAM_TOKEN_AMOUNT_WEI $PAIRED_TOKEN_AMOUNT_WEI $MIN_LIQUIDITY $LIQUIDITY_OWNER $ORDERS_CALLDATA $PREMINTS_CALLDATA"
print_verbose "Create calldata: $CREATE_CALLDATA"
CREATE_OUTPUT=$(sncast --account "$SNCAST_ACCOUNT" \
--wait --wait-timeout 120 \
invoke \
--url "$STARKNET_RPC" \
--contract-address "$STREAM_TOKEN_FACTORY" \
--function "create_token" \
--calldata $CREATE_CALLDATA 2>&1) || {
| pub const STREAM_DISTRIBUTIONS_NOT_STARTED: felt252 = 'Distributions not started'; | ||
| pub const STREAM_INVALID_ORDER_INDEX: felt252 = 'Invalid order index'; | ||
| pub const STREAM_POSITION_NOT_FOUND: felt252 = 'Position not found'; | ||
| pub const STREAM_INVALID_LIQUIDITY_OWNER: felt252 = 'Invalid liquidity owner'; |
There was a problem hiding this comment.
The new error constant is added correctly, but there should be a corresponding test in the test_constants.cairo file to verify the error message. While that file isn't modified in this PR, this is a gap in test coverage that should be addressed to maintain consistency with other error constants.
| fn get_liquidity_owner(self: @ComponentState<TContractState>) -> ContractAddress { | ||
| self.Stream_liquidity_owner.read() | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the get_liquidity_owner() getter function. Consider adding a test that verifies this function correctly returns the liquidity owner address that was set during initialization.
| // Transfer LP position NFT to liquidity owner | ||
| let nft_address = positions_dispatcher.get_nft_address(); | ||
| let nft = IERC721Dispatcher { contract_address: nft_address }; | ||
| let liquidity_owner = self.Stream_liquidity_owner.read(); | ||
| nft.transfer_from(get_contract_address(), liquidity_owner, position_id.into()); | ||
| self.emit(LiquidityPositionTransferred { position_id, owner: liquidity_owner }); |
There was a problem hiding this comment.
The NFT transfer happens after the state write and event emission (lines 296-307). This creates a potential inconsistency if the NFT transfer fails - the contract state would show liquidity was provided (state=1), but the owner wouldn't have received the NFT. Consider restructuring so the state write happens after the NFT transfer completes successfully to ensure atomic state consistency. This would mean moving the state write and LiquidityProvided event emission to after line 314.
| start_mock_call(MOCK_NFT(), selector!("transferFrom"), ()); | ||
| start_mock_call(MOCK_NFT(), selector!("transfer_from"), ()); | ||
| } | ||
|
|
||
| fn stop_mock_nft_transfer(mock_positions: starknet::ContractAddress) { | ||
| stop_mock_call(mock_positions, selector!("get_nft_address")); | ||
| stop_mock_call(mock_positions, selector!("transfer_from")); | ||
| stop_mock_call(MOCK_NFT(), selector!("transferFrom")); |
There was a problem hiding this comment.
The mock setup includes both transferFrom (camelCase) and transfer_from (snake_case) variants on lines 115-116. However, based on the actual code in stream.cairo line 313 and consistent patterns throughout the codebase (e.g., in ticket_booth.cairo, prize.cairo), only transfer_from (snake_case) is the correct Cairo convention. The camelCase variant appears to be unnecessary. Consider removing line 115 to avoid confusion and reduce test maintenance.
| start_mock_call(MOCK_NFT(), selector!("transferFrom"), ()); | |
| start_mock_call(MOCK_NFT(), selector!("transfer_from"), ()); | |
| } | |
| fn stop_mock_nft_transfer(mock_positions: starknet::ContractAddress) { | |
| stop_mock_call(mock_positions, selector!("get_nft_address")); | |
| stop_mock_call(mock_positions, selector!("transfer_from")); | |
| stop_mock_call(MOCK_NFT(), selector!("transferFrom")); | |
| start_mock_call(MOCK_NFT(), selector!("transfer_from"), ()); | |
| } | |
| fn stop_mock_nft_transfer(mock_positions: starknet::ContractAddress) { | |
| stop_mock_call(mock_positions, selector!("get_nft_address")); | |
| stop_mock_call(mock_positions, selector!("transfer_from")); |
| fn stop_mock_nft_transfer(mock_positions: starknet::ContractAddress) { | ||
| stop_mock_call(mock_positions, selector!("get_nft_address")); | ||
| stop_mock_call(mock_positions, selector!("transfer_from")); | ||
| stop_mock_call(MOCK_NFT(), selector!("transferFrom")); |
There was a problem hiding this comment.
Similar issue as above - line 122 stops mock for transferFrom (camelCase) but this is unnecessary. Only the snake_case variant transfer_from is used in the actual implementation. Consider removing line 122 to match the actual Cairo naming conventions.
| stop_mock_call(MOCK_NFT(), selector!("transferFrom")); |
Summary
liquidity_ownerfield toLiquidityConfigso the LP position NFT is transferred to a designated address after initial liquidity provision, instead of being permanently locked in the stream token contractprovide_initial_liquidity()mints the Ekubo LP position, the NFT is transferred viaIERC721::transfer_fromon Ekubo'sOwnedNFTcontract to theliquidity_ownerLiquidityPositionTransferredevent with position ID and owner addressChanges
Interface (
packages/interfaces/src/tokenomics/stream.cairo)liquidity_owner: ContractAddresstoLiquidityConfigstructget_liquidity_owner()toIStreamTokentraitStreamComponent (
packages/economy/src/tokenomics/stream/stream.cairo)IERC721Dispatcherimport for Ekubo NFT contractStream_liquidity_ownerstorage fieldLiquidityPositionTransferredeventinitializer()validatesliquidity_owner != zeroand stores itprovide_initial_liquidity()transfers LP NFT to owner after mintingget_liquidity_owner()getterSafety: The LP position ID (
Stream_liquidity_position_id) is only used by the read-only getterget_liquidity_position_id(). The TWAMM distribution positions (Stream_position_ids) are separate and remain in the contract forclaim_distribution_proceeds()to work. Transferring the LP NFT has no impact on distribution functionality.Tests (16 construction sites across 10 files)
LiquidityConfigstructs updated withliquidity_owner: OWNER()mock_nft_transfer/stop_mock_nft_transferhelpersLiquidityPositionTransferredevent assertion added to lifecycle events testScripts & Docs
scripts/create_stream_token.sh: Extract, validate, display, and serializeliquidity_ownerliquidity_ownerfieldAGENTS.md: UpdatedLiquidityConfigstruct documentationTest plan
scarb buildcompiles cleanlysnforge test --workspace— 2060 passed, 0 failed, 9 ignoredLiquidityPositionTransferredevent verified in lifecycle tests🤖 Generated with Claude Code