Skip to content
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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

AcrossfireX
Copy link

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.

@AcrossfireX
Copy link
Author

@FriendsFerdinand - FYI

@FriendsFerdinand
Copy link

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.

Copy link
Collaborator

@MarvinJanssen MarvinJanssen left a 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:

  1. vault-id is not introduced in the SIP. Can you explain the need and significance of this parameter?
  2. Did the SIP authors study existing on-chain vaults and base this SIP on their design? What were the conclusions?
  3. Is trait compatible with existing vaults or not? This can be mentioned in the "Backwards Compatibility" section.

@AcrossfireX
Copy link
Author

SIP has been updated to Accepted by SIP Editors

Comment on lines +46 to +54
`(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.
Copy link
Contributor

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?).

Comment on lines +66 to +68
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`.
Copy link
Contributor

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)?

Comment on lines +78 to +82
(asset-contract (uint) (response principal uint))

(holdings (uint) (response uint uint))

(holdings-of (uint principal) (response uint uint))
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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".

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?

Comment on lines 95 to 97
# 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
Copy link
Contributor

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
Copy link
Contributor

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?

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

@wileyj
Copy link
Contributor

wileyj commented Oct 27, 2023

I'm curious why the word "explorer" is used several times throughout the SIP.
It leads me to believe the end goal here is to both:

  1. add new vault trait
  2. display this trait in an explorer/api

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).
with the current wording, it makes it seem like changes to teh explorer/api would be necessary as part of this SIP.

)
```
# 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)
Copy link

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?

Choose a reason for hiding this comment

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

obycode added a commit that referenced this pull request Oct 27, 2023
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.

6 participants