-
Notifications
You must be signed in to change notification settings - Fork 162
NEP-621: Tokenized Vault #621
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
base: master
Are you sure you want to change the base?
Conversation
@near/nep-moderators This NEP is ready to be reviewed. |
Hi @edwardchew97, thank you for bringing new ideas and helping to push the entire ecosystem forward! As a NEP moderator, I reviewed your proposal, and there is only one thing I would like to ask you - please, remove the Other than that, everything looks fine, and as soon as you push the requested changes, I will ask WG to assign SMEs to review the proposal. |
Hi @garikbesson, thanks for pointing that out — I have removed it as requested. |
As a moderator, I'm asking @near/wg-contract-standards working group members to assign 2 Technical Reviewers to complete a technical review of the proposal. |
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.
Hi @edwardchew97 thanks for the good work. I have done a general review of the proposal (not the implementation) and compared it with original ERC4626 specification, and I have a couple of comments:
- The proposal is missing
deposit
andmint
methods. Is there any reason for this? AFAIK, these two methods are main state-changing flows in ERC4626. - In ERC4626,
totalAssets
view method states that it MUST be inclusive of any fees that are charged against assets in the Vault. Can you add this statement too? - Can you add the event emitted to the corresponding method description? For example,
withdraw
method should emitwithdraw
event. - Same you did for
max_redeem
, with "Implementations should return0
if...", can you add a similar statement formaxWithdraw
method as done in ERC4626? - Can you add when a method must or not revert (like it is done in the ERC4626)?
- Finally, in "Alternatives" it is mentioned this spec is not a "direct clone ERC-4626 clone without NEAR Adjustments". Are this adjustments purely in the implementation? If they affect the spec, can you add a section stating which methods differ from ERC-4626 spec and how?
@IkerAlus Thanks a lot for the thorough review! Addressed all your points below:
|
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.
hey @edwardchew97 thanks for the thorough and detailed answers. I think the spec looks almost ready to go. I only have one main comment, the rest are format changes and clarifications.
neps/nep-0621.md
Outdated
```` | ||
|
||
```rust | ||
pub trait VaultOptionalSlippageWithdraw { |
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.
are these two new methods mandatory in the NEP or just recommended?
I see they add a very valid use case but they are not part of ERC4626
neps/nep-0621.md
Outdated
|
||
````rust | ||
pub trait FungibleTokenReceiver { | ||
/// According to the NEP-141 standard, all fungible token transfers to a 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.
can we take out this explanation (from line 324 to 349) and place it just below "Deposit and Mint" subsection?
neps/nep-0621.md
Outdated
} | ||
``` | ||
|
||
Deposit and Mint |
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.
shouldn't this section be placed inside the "Contract Interface" section and not inside "Events" section?
neps/nep-0621.md
Outdated
/// correctly handle `ft_on_transfer` according to NEP-141: | ||
/// - Return `PromiseOrValue::Value(0)` if all tokens were accepted. | ||
/// - Return a non-zero `U128` to indicate the number of tokens to refund. | ||
fn ft_on_transfer(&mut self, sender_id: AccountId, amount: U128, msg: String) -> PromiseOrValue<U128> |
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.
Both deposit
and mint
should emit a VaultDeposit
event. Something you indeed do in the ref impl in https://github.com/Meteor-Wallet/tokenized-vault-nep-implementation/blob/5952f914a4f1601aa73ac142d77af45685309933/src/lib.rs#L276
This should be specified in the spec doc too for completeness.
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.
@edwardchew97 and @SteveKok great work with the specification of the NEP and thanks for addressing all my comments.
hey @frol @gagdiez can we find a couple of reviewers for the specification and also proposed implementation of this NEP?
@frol @robert-zaremba @fadeevab @mfornet Hi guys, The proposal is ready to be reviewed by SMEs, therefore, I'm asking @near/wg-contract-standards working group members to assign 2 Technical Reviewers to complete a technical review of the proposal. According to the NEPs Guidelines, if the reviewed NEP is not approved within two months, it is automatically rejected. For the presented proposal, this 2-month period starts from 15th August. |
I can have a look |
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.
Hi! I left two suggestions, check it out.
/// Returns the total amount of underlying assets represented by all shares in existence. | ||
/// | ||
/// **Important:** | ||
/// - Represents the vault's *total managed value*, not just assets held in the 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.
I understand where it comes from, but two "asset"/"assets" terms create a wild confusion. "Asset" means FT address, and "assets" means "amount of that one underlying asset (FT token)". Perhaps, it could be better addressed, e.g. "assets" could be called as "underlying asset's amount" in the docs comments.
neps/nep-0621.md
Outdated
/// Redeem a specific number of shares, **only if** they are worth | ||
/// at least `min_amount` of assets. | ||
/// This acts as a slippage-protected version of `redeem`. | ||
fn redeem_with_slippage( |
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.
It seems both redeem
and withdraw
must have slippage control via min_amount
parameter by default on NEAR, without "optional for security" redeem/withdraw_with_slippage
counterparts. Because NEAR is fundamentally asynchronous in execution, and because this NEP is adaptation to NEAR's reality, it's worth to introduce secure by default methods, not vice versa.
…with convert_to_assets, and assets params in preview_deposit
@SteveKok Thanks for these improvements! Now, we still have Take my suggestions about naming/renaming of assets with a grain of salt! Other than that, the NEP looks good 👍 |
@fadeevab thanks for raising this! After reviewing, I noticed that the
This leads me to think we should either:
After some digging, I realized we’ve actually been quite consistent conceptually:
So the keyword The real confusion, in my opinion, comes from the overlap with
That inconsistency between So I’d lean towards keeping
This way we keep both clarity and alignment with existing standards. |
…design name to specify the return value is shares or asset amount
@IkerAlus I noticed some differences between Solidity and Rust when defining interfaces. In Solidity, an interface allows us to assign names to return values, which serve as lightweight documentation and make it clearer what each value represents. In Rust traits, however, we can only specify the return type, without attaching descriptive names. Because of this, I’ve reworked many of the method names to be more self-explanatory, so that the intent of the return values is still clear even without named returns. I’d appreciate it if you could review the updated naming choices and share your feedback. |
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.
Reviewed a little bit more. Check other places in the documentation comments for inconsistencies.
hey @SteveKok Good point, I think it is a valid improvement and will help devs implementing and/on integrating with this contract standard. The NEP spec is all good after you fixed the inconsistencies as the last review pointed out. |
Rendered NEP: https://github.com/Meteor-Wallet/NEPs/blob/master/neps/nep-0621.md
Reference implementations: https://github.com/Meteor-Wallet/tokenized-vault-nep-implementation
NEP Status (Updated by NEP Moderators)
Status: Needs WG to assign SME