NEP-452 Linkdrop Standard #452
Conversation
|
Your Render PR Server URL is https://nomicon-pr-452.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cf7unio2i3mt56g24r7g. |
Co-authored-by: Marcelo Fornet <mfornet94@gmail.com>
There was a problem hiding this comment.
Thank you @BenKurrek for submitting this NEP.
As a moderator, I reviewed this NEP and it meets the proposed template guidelines. I am moving this NEP to the REVIEW stage and would like to ask the @near/wg-contract-standards and @near/wg-wallet-standards working group members to assign 2 Technical Reviewers (let's have one person with the Wallet expertise and one person with the Contracts expertise) to complete a technical review (see expectations). If you want to assign yourself, please mention that you are acting as the Technical Reviewers.
Please find some guidelines below for completing the technical review.
Technical Review Guidelines
-
First, review the proposal within one week. If you have any suggestions that could be fixed, leave them as comments to the author. It may take a couple of iterations to resolve any open comments.
-
Second, once all the suggestions are addressed, produce (example):
-
A recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation)
-
A summary of benefits
-
A summary of concerns and blockers that were identified on the way and their status (some will be resolved, others dismissed, etc)
-
Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again.
frol
added
WG-contract-standards
|
As a contract standards working group member, I definitely see the value in defining the linkdrop standard as it is widely used and previously we hacked things together into the NEAR Wallet, but other Wallets should also be able to onboard users through linkdrops. Adding NFT and FT support is also great quality of life improvement. |
|
As a wallet working group committee member, I’d like to formally nominate @joe-rlo from Ready Layer One Tech, who builds applications that implement LinkDrops to review NEP452: Linkdrop Standard as a Subject Matter Expect. |
|
As someone actively building multiple projects using first, linkdrop directly, and now through Keypom, determining a standard is needed at this time. The linkdrop mechanism is an important and powerful onboarding tool but is underutilized. In my experience, standardization is needed so more wallets can support this linkdrop and do it uniformly. While the mechanism is powerful, currently being limited by wallet support causes a disconnect between the goals of onboarding and decentralized choices. We are already seeing multiple wallets start to implement their own methods which as a developer will either lead to having to "pick a winner" or a need to support multiple methods. The clear benefits are as Ben stated:
The only concern I have is that I'd like to see the standard for deleting keys and refunding assets in the initial standard. The strength of having complex drops needs a standardized way to reclaim the assets as well. Overall, I think this is an important standard to begin to implement to onboard a billion users. |
|
As an update, I'm going to add an optional |
Co-authored-by: Benjamin Kurrek <57506486+BenKurrek@users.noreply.github.com>
|
I am self-assigning myself as an SME reviewer from the Contract Standards WG, so now we have two SMEs for this NEP. |
frol
removed
the
S-review/needs-wg-to-assign-sme
neps/nep-0452.md
Outdated
| 1. Linkdrop Creation | ||
| Linkdrop creation involves creating keypairs that, when used, have access to assets such as $NEAR, FTs, NFTs, etc. These keys should be limited access and restricted to specific functionality. For example, they should only have permission to call `claim` and `create_account_and_claim`. Since the keys allow the holder to sign transactions on behalf of the linkdrop contract, without the proper security measures, they could be used in a malicious manner (for example executing private methods or owner-only functions). | ||
|
|
||
| Another important security implication of linkdrop creation is to ensure that only one key is mapped to a set of assets at any given time. Externally, assets such as FTs, and NFTs belong to the overall linkdrop contract account rather than a specific access key. It is important to ensure that specific keys can only claim assets that they are mapped to. |
There was a problem hiding this comment.
If you add indentation (3 or 4 spaces) to this paragraph then you will fix the following markdown lint errors about list item enumeration
There was a problem hiding this comment.
@BenKurrek ☝️ finish those markdown lint errors please
There was a problem hiding this comment.
@BenKurrek see the summary of errors in the github action (at the bottom of the conversation tab)
There was a problem hiding this comment.
I will fix linting errors post consensus
Co-authored-by: Alexander Fadeev <fadeevab.com@gmail.com>
|
As a Contract Standards Working Group member, I lean towards approving this NEP. However, I would like the author to make certain small fixes should this NEP be approved. One is fixing the Markdown formatting. Another one is clarifying the potential security implications of race conditions (one claim call happening while another is in the process of transferring tokens), attempting to create a linkdrop with a key that corresponds to an already existing linkdrop (if this is not checked, an attacker could overwrite existing linkdrops without knowing their private key), and the combination of these two (attempting to create a linkdrop with a key that corresponds to an already existing linkdrop, which is in the process of transferring tokens). |
I've addressed both the reentrancy attacks as well as key management (making sure only one key is mapped to a set of assets at a given time) in the security implications section. I will fix the markdown issues and linting post consensus. |
I don't see any mention of an attack where an attacker attempts to create a linkdrop with the same key as an existing one. |
Line 195 in 17c0294 |
|
"only one key is mapped to a set of assets" != "only one set of assets is mapped to a key" |
|
The Contract Standards Work Group met last Friday to review this NEP and reached the following consensus: Status: Delay decision due to an unresolved concern. Summary: Meeting Recording: @BenKurrek @kenobijon Thank you for authoring this NEP! Next Steps:
|
There was a problem hiding this comment.
There are still few unaddressed points from the original review: #452 (review)
| 3. Asset Refunds & Failed Claims | ||
| Given that linkdrops could contain multiple different assets such as NFTs, or fungible tokens, sending assets might happen across multiple blocks. If the claim was unsuccessful (such as passing in an invalid account ID), it is important to ensure that all state is properly managed and assets are optionally refunded depending on the linkdrop contract's implementation. | ||
|
|
||
| 4. Fungible Tokens & Future Data |
There was a problem hiding this comment.
| 4. Fungible Tokens & Future Data | |
| 4. Storage Registration & Future Data |
There was a problem hiding this comment.
This is not specific to Fungible Tokens. Can also be related to NFTs or Multitoken... Let's just call it Storage Registration (which is a standard by it's own).
| Given that linkdrops could contain multiple different assets such as NFTs, or fungible tokens, sending assets might happen across multiple blocks. If the claim was unsuccessful (such as passing in an invalid account ID), it is important to ensure that all state is properly managed and assets are optionally refunded depending on the linkdrop contract's implementation. | ||
|
|
||
| 4. Fungible Tokens & Future Data | ||
| Fungible token contracts require that anyone receiving tokens must be registered. For this reason, it is important to ensure that storage for accounts claiming linkdrops is paid for. This concept can be extended to any future data types that may be added. You must ensure that all the pre-requisite conditions have been met for the asset that is being transferred. |
There was a problem hiding this comment.
ensure that storage for accounts claiming linkdrops is paid for
That may not be possible, because if it's a new account claiming a drop, then we don't know the new account address.
|
What is blocking this from moving forward? |
|
As a Contract Standards Group member, I approve this PR. |
|
As noted above, I would prefer that the claim methods returns object with |
|
BTW, would be good to cleanup the comments, and check the description improvements before merging. |
|
@BenKurrek Please, address the markdown lints and remaining comments from @robert-zaremba:
@fadeevab Please, add Changelog section with the benefits and concerns mentioned in the reviews (see NEP-399 for example). |
|
@frol I've added the final touches to the NEP as requested by you. Is it ok to move forward at this point? |
|
@BenKurrek All looks good except the markdown-lint. Please, fix CI, and I will add the changelog and merge this NEP! |
|
The lint issues is fixed here |
Discussions found here.
NEP Status (Updated by NEP moderators)
SME reviews:
Contract Standards WG voting indications:
Wallet Standards WG voting indications: