Implement access controls for Position objects#130
Conversation
Convert Position from struct to resource type with entitlements-based access control. This addresses the TODO at line 3596 and the security concern at line 2640 regarding position access permissions. Key changes: - Added new position-specific entitlements (EPositionDeposit, EPositionWithdraw, EPositionConfigure, EPositionManage) - Converted Position struct to resource type with fine-grained entitlement controls - Updated Pool.createPosition() to return @position resource instead of UInt64 - Added storage path helper functions (getPositionStoragePath, getPositionPublicPath) - Removed PositionWrapper pattern in favor of direct Position resource storage - Updated all test transactions to work with Position resources - Position resources are now held in user accounts with proper ownership Breaking changes: - Pool.createPosition() returns @position instead of UInt64 - Position can no longer be manually constructed - All position operations require appropriate entitlements - Users must store Position resources in their accounts https://claude.ai/code/session_01QbCEFZ42NrxU7hdZo7KWPw
Resolved conflicts in the following files: - cadence/tests/funds_available_above_target_health_test.cdc:11-17 Reasoning: Redundant variable declarations that were moved to test_helpers.cdc Resolution: Removed local declarations (flowTokenIdentifier, moetTokenIdentifier, flowVaultStoragePath) as they are now imported from test_helpers.cdc - cadence/tests/funds_required_for_target_health_test.cdc:12-18 Reasoning: Same refactoring - constants moved to shared test_helpers.cdc Resolution: Removed local declarations in favor of imported constants from test_helpers.cdc - cadence/tests/position_lifecycle_happy_test.cdc:15-20 Reasoning: Same refactoring - constants moved to shared test_helpers.cdc Resolution: Removed local declarations (flowTokenIdentifier, flowVaultStoragePath) in favor of imported constants - cadence/tests/position_lifecycle_happy_test.cdc:88-92 Reasoning: Conflicting transaction parameter - positionId (HEAD) vs WRAPPER_STORAGE_PATH (origin/main) Resolution: Kept positionId parameter from HEAD, as the transaction file (repay_and_close_position.cdc:18) expects positionId: UInt64 parameter, not a storage path Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Test transactions now borrow Position references directly from PositionManager instead of calling proxy methods. This follows the pattern where PositionManager only provides core collection functionality (add, remove, borrow) and users interact directly with Position objects. Changes: - borrow_from_position.cdc: Borrow Position and call withdraw() directly - deposit_to_wrapped_position.cdc: Borrow Position, auto-find position ID, and call depositAndPush() directly - repay_and_close_position.cdc: Borrow Position and call methods directly - flow.json: Add missing test contracts (UniswapV2SwapConnectors, TokenA, TokenB, EVMAbiHelpers) All 31 tests now passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add createPositionPublic() wrapper function to Pool that allows anyone to create positions without requiring beta grant capabilities. Remove MockFlowCreditMarketConsumer infrastructure and replace with direct Position operations via PositionManager. All tests passing (31/31). Changes: - Add Pool.createPositionPublic() public wrapper function - Remove MockFlowCreditMarketConsumer contract and transactions - Add position-manager transactions for deposit/borrow operations - Update all tests to use public creation pattern - Remove grantPoolCapToConsumer() helper function Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace the wrapper function approach with a simpler solution: change createPosition() from access(EParticipant) to access(all). This allows the function to be called through the existing public Pool capability without requiring a wrapper or attempting to publish entitled capabilities (which Cadence runtime prevents). Key insight: Cadence prevents publishing auth(E) capabilities to public paths at runtime, so the cleaner approach is to change the function's access level directly rather than working around it with wrappers. Changes: - Change Pool.createPosition() from access(EParticipant) to access(all) - Remove createPositionPublic() wrapper function (no longer needed) - Update transaction to use standard PoolPublicPath - All 31 tests passing Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Update Position storage paths to camelCase with address suffix - Remove EPositionDeposit entitlement (deposits now access(all)) - Replace EPositionWithdraw with FungibleToken.Withdraw - Remove EpositionAdmin entitlement (unused) - Add documentation for EPositionManage entitlement - Remove beta grant references from documentation - Clean up changeset-specific comments Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Reorganize transaction files by moving create_position.cdc and repay_and_close_position.cdc from tests/transactions/pool-management to transactions/flow-credit-market/position/. Update all test file references to use the new location. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
jordanschalm
force-pushed
the
jord/115-position-permissions
branch
from
a3e2901 to
e55c60f
Compare
jordanschalm
changed the title
| /// Position resources are held in user accounts and provide access to one position (by pid). | ||
| /// Clients are recommended to use PositionManager to manage access to Positions. | ||
| /// | ||
| access(all) resource Position { |
There was a problem hiding this comment.
So happy this is being converted to a resource. I tried to get them to do that last year but they said it wasn't a priority at that moment
There was a problem hiding this comment.
Can we talk about this at the sync tomorrow? It was my design decision to make it a struct, and it wasn't an accident.
It's quite possible that making it a struct is more trouble/confusion than it's worth, but it was my firm belief that it makes it easier to build on top of of the lending protocol while still being secure. I haven't been working on the code, so my mind is open, but I would like to understand the logic behind this change.
There was a problem hiding this comment.
Yeah, let's discuss the details tomorrow. The main goal here is implementing per-user access control (I make a Position, then only I can withdraw/modify it). I chose a resource to match other places where a resource represents user-specific access (NodeStaker, DKGParticipant, etc.)
We are aiming to have per-user access control for positions merged prior to the next audit round (next Monday).
There was a problem hiding this comment.
Decided to keep this as resource at Q&A meeting today Feb 5.
- There is no functionality that can only be implementing using a struct -- it purely simplifies implementing some functionality
- We would like to implement a
duplicatefunction on thePositionresource to simplify the process of sharing it with other systems (generally useful feature for these kinds of "access key" resources), later.
| /// A collection resource that manages multiple Position resources for an account. | ||
| /// This allows users to have multiple positions while using a single, constant storage path. | ||
| /// | ||
| access(all) resource PositionManager { |
There was a problem hiding this comment.
I think I brought this up before, but would it make sense to make Position and NFT and PositionManager an NFT Collection? Just to plug into all the NFT functionality and interfaces that already exist? It is different enough that I could see that not being useful though, but curious to hear others thoughts
There was a problem hiding this comment.
I considered this when working on the first draft of this code, and decided against it, but I'm open to hear other folks' thoughts on this.
I heard there were some problems when Uniswap switched to using NFTs to represent LP positions in v3. Most people think of "NFT" as meaning "a collectible", and a number of people sold their Uniswap NFTs thinking that it was just a "free gift" for being an LP instead of being the actual object that represents their LP position.
From a software abstraction mindset, I can see how making it an NFT is appealing, and allows us to share some code, but from a user abstraction mindset, it seems like it could create confusion and problems. Do we really need people seeing their lending positions on the NFT page of their wallet? Do we want them to bridge them to EVM, and list them on OpenSea? Maybe yes? But I personally think the problems that can come up outweigh the advantages.
Having said that, I have talked to Jeff about creating a new interface (or at least new metadata types) that represent defi accounts/positions/investments so that wallets CAN make it easier for users to manage them. There could be a single page in your wallet that shows all of your various investments and available funds (including credit lines). We don't really have the bandwidth to work on that at the moment, but I personally think that's a much better fit than using the NFT interface.
There was a problem hiding this comment.
Personally, I've gone down that route of issuing an NFT for a position previously and noticed that it didn't really improve functionality that much without spending a lot of additional effort for all of the downhill integrations or actions that you thought would occur. Also for all of the "wallet viewers" they would have to create custom tooling to digest that position data.
The big benefit from an end user perspective would be being able to take additional leverage or rehypothecation. While allowing for that opens a lot of additional tailrisk imo and adds complexity for not too much gain.
There was a problem hiding this comment.
Decided to not make Position implement NFT interface at Q&A meeting today Feb 5. Consider implementing DeFi-specific metadata information for Position in the future.
| /// Grants access to configure drawdown sinks, top-up sources, and other position settings, for the Position resource. | ||
| /// Withdrawal access is provided using FungibleToken.Withdraw. | ||
| access(all) entitlement EPositionManage |
There was a problem hiding this comment.
nit: rename to EPositionAdmin or EPositionControl to avoid confusion with PositionManager
| /// Returns a Position resource that provides fine-grained access control through entitlements. | ||
| /// The caller must store the Position resource in their account and manage access to it. | ||
| /// Clients are recommended to use the PositionManager collection type to manage their Positions. | ||
| access(all) fun createPosition( |
There was a problem hiding this comment.
restore access gating
| access(all) fun createPosition( | |
| access(EParticipant) fun createPosition( |
There was a problem hiding this comment.
Thanks - I didn't realize this was going to be used for beta access gating, but that makes sense. I have reverted this change so position creation is gated again.
nialexsan
left a comment
nialexsan
left a comment
There was a problem hiding this comment.
it looks like this PR removed gated access to position creation
probably it should be restored, since the project is still in closed beta
|
@nialexsan Thanks for the review - I revert the removal of beta access gating and addressed other comments |
6 participants
Ref: #115
This PR adds access control for
Positionobjects.Positionstruct to resource typeEPositionManageentitlement protects access to sensitive configuration change methdosFungibleToken.Withdrawentitlement protects access to withdrawalsPosition, maintain current policy of anyone can deposit to any positionPositionManager, which acts as a collection type forPosition's.createPositionreturns aPositionresource rather than a position IDcreatePositionis also nowaccess(all).