Conversation
91e5ba0 to
dc4ef93
Compare
dc4ef93 to
5caf5a9
Compare
|
@holyfuchs I am still reading the PR (also using this as an opportunity to familiarize myself with Cadence a bit more). I noticed that the high-level design intent (identity-based gating, non-transferable permission, the wrapper pattern) isn't documented anywhere in the contract code. The spec covers the formal logic but the PR description's "How it works" narrative — which has actually been the most useful explanation for me — seems to be outdated (?). But most importantly, context you are providing to a reviewer in form of PR descriptions should be part of the implementation as well. AI can create documentation, don't skimp on it. My recommendation
Of course you are dealing with very simple logic. But let's also use this as a learning opportunity and practise the patterns like documentation that we want to employ in more complex scenarios. Clearly documenting intend and logic is one of the most valuable approaches to reduce error probability in my opinion. |
AlexHentschel
left a comment
There was a problem hiding this comment.
First round of feedback. Mainly focused on the spec
|
|
||
| ### Primitives & Notation | ||
| * **$E$**: The set of addresses with Early Access (the "Allowlist"). | ||
| * **$s$**: The transaction signer address. |
There was a problem hiding this comment.
| * **$s$**: The transaction signer address. | |
| * **$s$**: The transaction's signer address. |
| * **$Cap(s, V)$**: A boolean representing if $s$ has a valid Cadence-level right to $V$ (meaning $s$ is the **Owner** OR holds a **Capability**). | ||
| * **$op(s, V)$**: Signer $s$ attempting an operation (Deposit/Withdraw) on $V$. |
There was a problem hiding this comment.
this is rendered as Latex Code, and latex interprets \texttt, which prints them in a mono-space typewriter font, which many people intuitively associate with shell functions.
| * **$Cap(s, V)$**: A boolean representing if $s$ has a valid Cadence-level right to $V$ (meaning $s$ is the **Owner** OR holds a **Capability**). | |
| * **$op(s, V)$**: Signer $s$ attempting an operation (Deposit/Withdraw) on $V$. | |
| * **$\texttt{Cap}(s, V)$**: A boolean function representing if $s$ has a valid Cadence-level right to $V$ (meaning $s$ is the **Owner** OR holds a **Capability**). | |
| * **$\texttt{op}(s, V)$**: Signer $s$ attempting an operation (Deposit/Withdraw) on $V$. |
|
|
||
| --- | ||
|
|
||
| ### Case Matrix |
There was a problem hiding this comment.
Who need to authorize the creation of a position is fundamentally different from who is allowed to modify a position (deposit / withdraw). You may conclude that we can handle both through the same mechanism, but at the moment, you are not even discussing this fundamental difference from a security perspective.
| | $op(B, V_1)$ | **Abort** | $s \notin E \wedge \neg Cap(B, V_1)$ | | ||
| | $op(B, V_2)$ | **Abort** | $s \notin E$ | |
There was a problem hiding this comment.
you have introduced the symbols
| * **Storage:** $E$ is represented by `map: {Address: Bool}`. | ||
| * **Entry Point:** Checked via `fun protectedFunction(signer: &Account)`. |
There was a problem hiding this comment.
Here, you should be providing more detail. Follow up on the fact that creation of a position requires fundamentally different authority from who is allowed to modify a position (deposit / withdraw). In each case, describe how we intend to enforce in the implementation that only the correctly authorized party is successful.
Comment or describe APIs please - they are technically part of the spec and cannot be freely chose by the implementation (without impacting other components)
7a280cf to
7d716e9
Compare
8f2ab1b to
2b7ac28
Compare
| if FlowYieldVaultsEarlyAccess.checkPass(addr: addr) { | ||
| let pass = FlowYieldVaultsEarlyAccess.borrowPass(addr: addr) |
There was a problem hiding this comment.
Maybe just have borrowPass return an optional instead of panicing, and then checking is not needed here
| /// Address this pass was issued to; stamped at creation and never changes. | ||
| access(all) let addr: Address |
There was a problem hiding this comment.
I guess this address is purely informational for off-chain purposes? Is it actually needed off-chain?
| /// Returns whether a pass is currently held for the given address. | ||
| /// | ||
| /// **Parameters** | ||
| /// - `addr`: Recipient to check. | ||
| /// | ||
| /// **Returns** `true` if a pass exists for `addr`, `false` otherwise. | ||
| view access(all) fun passExists(addr: Address): Bool { | ||
| return self.checkPass(addr: addr) | ||
| } |
There was a problem hiding this comment.
This function seems redundant. However, passExists is IMHO better named than checkPass (makes me wonder: what does it "check"?). So maybe just inline checkPass here and switch all existing uses of checkPass to passExists
| } | ||
|
|
||
| access(self) fun loadPass(addr: Address): @EarlyAccessPass { | ||
| return <- (self.account.storage.load<@EarlyAccessPass>(from: self.passStoragePath(addr: addr)) ?? panic("Pass not found")) |
There was a problem hiding this comment.
Here for loading and below for borrowing: I'd just return an optional and have the caller decide how to handle non-existence.
| } | ||
|
|
||
| view access(self) fun passStoragePath(addr: Address): StoragePath { | ||
| return StoragePath(identifier: "FlowYieldVaultsEarlyAccessPass_\(addr.toString())")! |
There was a problem hiding this comment.
Not sure, but I think an Address can be used directly as a string template expression:
| return StoragePath(identifier: "FlowYieldVaultsEarlyAccessPass_\(addr.toString())")! | |
| return StoragePath(identifier: "FlowYieldVaultsEarlyAccessPass_\(addr)")! |
| | Term | Definition | | ||
| | :--- | :--------- | | ||
| | **Vault** (`YieldVault`) | A Cadence resource representing a yield-generating position. | | ||
| | **Pass** (`EarlyAccessPass`) | A Cadence resource stored in contract account storage. Represents a grant of vault-creation rights. Never held by the user directly. Keyed by recipient address: each address has at most one pass at a time. | |
There was a problem hiding this comment.
Maybe note here in which account these are stored. Are they first stored in the contract account, then transferred to the participant?
| /// Storage path where pass capabilities are stored for claiming. | ||
| access(all) let passCapabilityStoragePath: StoragePath | ||
|
|
||
| /// Held in the holder's storage; gates yield vault creation during early access. |
There was a problem hiding this comment.
| /// Held in the holder's storage; gates yield vault creation during early access. | |
| /// Held in the admin's storage; capability granted to user; gates yield vault creation during early access. |
Flow Yield Vaults — Early Access
Overview
Problem
During the launch phase, yield vault creation must be restricted to vetted participants. An open deployment would expose the protocol to malicious actors before the system has been stress-tested in production. Participants are semi-trusted — we can reasonably assume they will not behave maliciously.
Goal
Gate yield vault creation behind an allowlist curated by us. Each approved participant receives one
EarlyAccessPasswith an allowance — a fixed number of yield vaults they may create. One pass equals one participant; the allowance is the cap on how many vaults that participant can open. If a participant misbehaves, the admin revokes their pass, immediately blocking any further vault creation. This limits exposure in the event of a leaked capability: the blast radius is bounded by the allowance on that specific pass and the capital permitted per vault.Lifetime
This is a temporary restriction.
FlowYieldVaultsEarlyAccessis a thin wrapper over the core vault interfaces with no state in the underlying contracts. Removing early access requires updating the contract that implementsFlowYieldVaultsInterfaceswithout theaccess(account)gate onfun createYieldVault. After that, vault creation is open to anyone — no pass and no allowance required.FlowYieldVaultsEarlyAccessremains deployed but becomes a dead entrypoint; existing passes are irrelevant since users will interact with the new open contract directly.Check /docs for further information.