-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sip 026 - Clarity DeFi Vault SIP #153
base: main
Are you sure you want to change the base?
Conversation
@FriendsFerdinand - FYI |
The main reasoning behind this proposal is for explorers and other APIs to have standardized access to the information to vault state that is stored in a single Clarity contract. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial requests for clarification:
vault-id
is not introduced in the SIP. Can you explain the need and significance of this parameter?- Did the SIP authors study existing on-chain vaults and base this SIP on their design? What were the conclusions?
- Is trait compatible with existing vaults or not? This can be mentioned in the "Backwards Compatibility" section.
SIP has been updated to Accepted by SIP Editors |
`(asset-contract ((vault-id uint)) (response principal uint))` | ||
|
||
Returns the principal of the asset being held by the vault identified by `vault-id`. | ||
|
||
Returns the token type balance `token-id` of a specific principal `who` as an | ||
unsigned integer wrapped in an `ok` response. It has to respond with `u0` if the | ||
principal does not have a balance of the specified token or if no token with | ||
`token-id` exists. The function should never return an `err` response and is | ||
recommended to be defined as read-only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this function isn't super clear to me from this description.
I think it is supposed to return the contract that controls the asset in the given vault, but the description of the function here seems like it maybe is describing something else (i.e., what is the token type balance
referring to?).
Returns the total amount of the underlying asset held by the vault identified by | ||
their vault ID for `owner`. This is used when underlying assets are divided by vault id | ||
and are grouped by the principal of `owner`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This SIP needs more detail on the difference between "normal vaults" and "vaults grouped by principal". It's not clear in the text what the difference between those models is and what should be possible. For example does it make sense to use the same vault id with two different principals? That is, could (holdings-of u10 <asset> alice)
return (ok u10)
and also (holdings-of u10 <asset> bob)
return (ok u5)
?
(asset-contract (uint) (response principal uint)) | ||
|
||
(holdings (uint) (response uint uint)) | ||
|
||
(holdings-of (uint principal) (response uint uint)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest all of the trait functions be prefaced with vault-
because they have to share the function namespace with the rest of the contract.
|
||
However, Clarity's design differs significantly. It does not allow contract deployment during transaction execution to maintain deterministic behavior and avoid infinite recursion. Consequently, representing entities that hold SIP-010 assets requires implementing logic within a single contract address. | ||
|
||
# Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to talk about event production (via print
) for vaults.
I imagine that a standardized deposit/withdraw event for vaults could eventually be used by explorers to display a more detailed event outcome for a transaction. For example, rather than transfered FOO to SP1VFTXPFH4S8T99NHWQBJ8BECKZFQ3YFXRN4NJY8.bns
it could say transfered FOO to SP1VFTXPFH4S8T99NHWQBJ8BECKZFQ3YFXRN4NJY8.bns, depositing in vault 10
or whatever. But it can only do that if the vault events are standardized.
|
||
However, Clarity's design differs significantly. It does not allow contract deployment during transaction execution to maintain deterministic behavior and avoid infinite recursion. Consequently, representing entities that hold SIP-010 assets requires implementing logic within a single contract address. | ||
|
||
# Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the SIP would be improved with 2 simple example contracts that implement "normal vaults" and "vaults grouped by principal".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should they be included in the SIP or linked to in the SIP?
# Reference Implementation | ||
The reference implementation of this SIP can be found in the following GitHub repository: | ||
GitHub: https://github.com/FriendsFerdinand/sips/blob/vault-standard/sips/sip-vault/sip-vault.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a dead link. In my opinion, because this SIP is a trait proposal, it is probably sufficient just to include the text of the trait definition here.
|
||
# Introduction | ||
|
||
#### Problem Statement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When mentioning how Ethereum has resolved this problem, can you add a link to an exmple contract/etherscan tx etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example of a pool deployed by a transaction: https://etherscan.io/address/0xc1dd3f011290f212227170f0d02f511ebf57e433#code
Users can mint, burn and deploy assets to the pool and can follow the assets stored in the pool by looking at the balance in the explorer.
As a result, users who post collateral on Stacks DeFi apps might perceive their collateral funds as commingled with other users' collateral, leading to a suboptimal user experience. The ability to natively inspect collateral positions on-chain is a crucial value proposition of DeFi over CeFi. | ||
|
||
#### Technical Background of the Problem | ||
In Solidity, the Ethereum smart contract language, contracts can be deployed by executing Solidity code in transactions. This allows the deployment of contracts that represent vaults holding ERC-20s separately. These distinct collateral-holding contracts can be effortlessly looked up on explorers such as Etherscan, facilitating users' oversight of DeFi protocol collateralization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Solidity, the Ethereum smart contract language, contracts can be deployed by executing Solidity code in transactions. This allows the deployment of contracts that represent vaults holding ERC-20s separately. These distinct collateral-holding contracts can be effortlessly looked up on explorers such as Etherscan, facilitating users' oversight of DeFi protocol collateralization. | |
In Ethereum's smart contract language Solidity, contracts can be deployed by executing Solidity code in transactions. This allows the deployment of contracts that represent vaults holding ERC-20s separately. These distinct collateral-holding contracts can be effortlessly looked up on explorers such as Etherscan, facilitating users' oversight of DeFi protocol collateralization. |
I'm curious why the word "explorer" is used several times throughout the SIP.
But, it's a bit unclear how the explorer would do this as it pertains to the SIP. I think if the goal here is to add a new trait, language around how the explorer may use this should be more generic (ex: stacks explorers would then have the ability to display xyz in an easily digested manner). |
) | ||
``` | ||
# Related Work | ||
The dicussion on a vault Solidity implementation can be found here [Forum Discussion](https://www.usenix.org/conference/atc16/technical-sessions/presentation/ali) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right link? Doesn't link to any Solidity example but a dated Blockstack paper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake. I mean to link this: https://ethereum-magicians.org/t/eip-4626-yield-bearing-vault-standard/7900
Here is the initial draft of SIP-026 for Clarity Defi Vaults. After initial review by SIP editors we have the following RICE score.
% of network affected: 90%
Reach: 150,000
Impact: 2
Confidence: 80%
Effort: 2
RICE Score: 120,000
This scoring can change with additional information as its available.