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

CPS-0009? | Coin Selection Including Native Tokens #611

Merged

Conversation

HinsonSIDAN
Copy link
Contributor

@HinsonSIDAN HinsonSIDAN commented Oct 23, 2023

This seems a long withstanding issue #232 where most offline library we got now still using sub-optimal coin selection strategy which might lead to inefficiency transaction building process.

Raising this CPS in an attempt to attract community's attraction to solve this issue with combined intelligence.


Recreating the PR #609


(rendered proposal in branch)

@rphair rphair added the Category: Wallets Proposals belonging to the 'Wallets' category. label Oct 24, 2023
@rphair rphair changed the title CPS-???? - Coin Selection Approach for Post Native Tokens Era CPS-???? | Coin Selection Including Native Tokens Oct 24, 2023
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@SIDANWhatever thanks for your effort to resubmit this with permissions that will allow more interactive editing with editors (I hope to document some standard means of doing this when we get a CIP Wiki together in the next few months).

@nicolasayotte @siegfried can you have a look at this document and let us know what you think? ... particularly, are there any commonly agreed upon problem definitions & solutions that could be paraphrased from #232 into this CPS?

  • Other editors: What level of detail might be appropriate for those coin selection particulars to be included in the CPS itself? Or is this the kind of thing that we would leave entirely to CIPs linked to this CPS?

Looking forward to discussing both scope & content questions at the next CIP meeting & have put it on the agenda: https://hackmd.io/@cip-editors/76

CPS-????/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Further editorial formalities (still looking forward to detailed review of content itself).

CPS-????/README.md Outdated Show resolved Hide resolved
CPS-????/README.md Outdated Show resolved Hide resolved
@Crypto2099
Copy link
Collaborator

I love the concept of this CPS and think it is a great candidate to house and centralize conversation around the topic of coin selection algorithms in general. I would like to also suggest that we include research and solutions into better change creation algorithms as well as this is a large part of my focus on my own UnFrack.it dApp microservice and the library I intend to create utilizing it.

I would love to see this CPS serve as a rallying point for any/all new CIPs that may suggest a new, purpose-built/targeted coin selection or changing algorithm so that those interested parties have an easy path to discovering what conversations, tools, and algorithms may already exist to solve their unique situational problems.

@nicolasayotte
Copy link
Contributor

nicolasayotte commented Nov 13, 2023

The document seems like a very good statement of the issue. The point is that the most used libraries should come with a coin selection that follows a set of high quality standards that work within the current eUTxO ecosystem with native tokens and NFTs. :)

I think that the cardano-wallet explanation of the algorithms of Jonathan Knowles was very good and could be taken as a basis for updates in serialization librairies.

@michaelpj
Copy link
Contributor

I'm unsure what a solution to this problem would look like. Are we looking for:

  1. A specification of an implemention-independent coin-selection algorithm that everyone can implement?
  2. One or more implementations of the algorithm?

@HinsonSIDAN
Copy link
Contributor Author

HinsonSIDAN commented Nov 14, 2023

  1. A specification of an implemention-independent coin-selection algorithm that everyone can implement?

I think the outcome could be something like an informative CIP like the existing CIP-2, which it clearly suggests best practices. It could include specific algorithm implementation or just outlining the logic of the desired algorithm.

  1. One or more implementations of the algorithm?

Yea it could be, just like currently we have both the largest coin comes first & random selection. Multiple algorithms could be a solution. At the end it is up to the off-chain code builders to choose which to rely on / implementing both.

@rphair rphair changed the title CPS-???? | Coin Selection Including Native Tokens CPS-0009? | Coin Selection Including Native Tokens Nov 15, 2023
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Thanks @SIDANWhatever - as you know we agreed to promote this to CPS candidate at the meeting today, so please update the document & the "rendered proposal" link in your original posting.

As I mentioned in the meeting today I believe it would be instructive to mention @siegfried's https://www.npmjs.com/package/cardano-utxo-wasm to include / compare / contrast with other approaches being considered here.

CPS-????/README.md Outdated Show resolved Hide resolved
@rphair
Copy link
Collaborator

rphair commented Nov 27, 2023

a note from today's Wallet Connectors Workshop (more meetings to follow; announced on CIP Editors' Discord): the matter of coin selection was identified during the meeting as an important issue.

@siegfried
Copy link

I don't have much to add here. Just to mention the algorithm I used resolved our selection problem for RoundTable and it works well so far. If you have better options please let me know.

@HinsonSIDAN
Copy link
Contributor Author

From meeting, 2 followups for my side:

  1. Including change utxo as scope on this CPS
  2. Add a short session documenting all existing implementation, so that everyone could add on the list & reduce research time for potential CIP proposers


Decentralized applications and DeFi projects require efficient coin selection to maintain the performance and cost-effectiveness of their transactions.

### Example Implementation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is the place to add this block as we discussed in biweekly. Also if anyone knows the right link pointing to the implementation for cardano wallet please help to supplement!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this makes perfect sense because Example Implementations will always be closely related to, if not qualify as, Use Cases.

Copy link
Collaborator

@rphair rphair Nov 28, 2023

Choose a reason for hiding this comment

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

so far I think this list is:
👉 the dev version of cardano-wallet @nicolasayotte mentioned in today's meeting
👉 anything open source from what @siegfried mentioned in #611 (comment)
👉 cardano-utxo-wasm as per #611 (review)

@rphair
Copy link
Collaborator

rphair commented Nov 28, 2023

@SIDANWhatever noting from our meeting today that we believe this is ready to mark Last Check pending your inventory that you are working on as per #611 (comment) and #611 (comment).

@rphair rphair added the State: Last Check Review favourable with disputes resolved; staged for merging. label Nov 28, 2023
@HinsonSIDAN
Copy link
Contributor Author

HinsonSIDAN commented Nov 29, 2023

@SIDANWhatever noting from our meeting today that we believe this is ready to mark Last Check pending your inventory that you are working on as per #611 (comment) and #611 (comment).

@rphair The 2 followups are performed. Please check if there is other things to supplement. I think we could proceed with the final check.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Looks good so far; I had to make some grammar adjustments which I hope will be subjected to further review especially as they might affect meaning... particularly in the cases whether or not we are ultimately seeking 1 single ideal coin selection algorithm.

I'm approving pending 1 more single update which we should all check for before merging...

CPS-0009/README.md Show resolved Hide resolved
Copy link
Collaborator

@Ryun1 Ryun1 left a comment

Choose a reason for hiding this comment

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

Hey @SIDANWhatever, thanks for this problem statement - I am quite a fan of it.

Had a few small review comments below.

CPS-0009/README.md Outdated Show resolved Hide resolved
CPS-0009/README.md Outdated Show resolved Hide resolved
CPS-0009/README.md Outdated Show resolved Hide resolved
CPS-0009/README.md Outdated Show resolved Hide resolved
@rphair
Copy link
Collaborator

rphair commented Dec 12, 2023

@SIDANWhatever we had a window to merge this at Last Check in the meeting today but could not do so without a response yet to @Ryun1's #611 (review) which will require at least one change to the text.

Our next CIP meeting isn't for another 4 weeks, so in my opinion we should be able to merge this in the meantime if you make those responses / updates and then with editor unanimity we can follow through from GitHub alone, since there was a call for "objections" at today's meeting & there were none.

@HinsonSIDAN
Copy link
Contributor Author

@rphair The replies and those final checks are done. After final review from our side, we also added useful change outputs consideration into the CPS (which exists on CIP2). Let me know if there is any further follow-ups!

@fallen-icarus
Copy link

fallen-icarus commented Dec 13, 2023

@SIDANWhatever Apologies for the last minute comment... There is also a security risk that should be addressed by any coin selection method with respect to DeFi DApps. For example, with my cardano-loans, an NFT is used to represent the lender. In order to do any lender related actions (eg, update the address where payments must go, claim expired collateral, etc), the NFT must be in one of the inputs to the transaction (not a reference input). This NFT should never be part of any transaction that does not involve the corresponding loan. Since the NFT can only be "spent" (even back to the same owner) if the owner approves with a signature or script logic, the spending of the NFT is considered consent to do something with the loan. It is effectively a proxy for the lender's approval.

There are benefits to using NFTs like this. Consider if Bob currently owns the NFT for a loan but doesn't want it anymore. He can put it up for sale on the secondary market. Charlie can see the NFT and satisfy the conditions of the secondary market in order to claim the NFT. Because the NFT is "in motion" (the UTxO with it is being consumed), Charlie is able to update the payment address for the loan in the same transaction where he purchases the NFT. This allows Charlie to not miss the next payment made on the loan. It is likely not possible for Bob to sign Charlie's transaction directly so the NFT being "in motion" is the only way for Bob to express consent in a trustless and p2p fashion. To be concrete, how would this hand-off occur if the loan DApp required Bob to approve of Charlie's change of address with Bob's signature? This transaction would require both Bob's signature and Charlie's signature; this is likely not possible without a trusted intermediary.

If another DApp is malicious and sees that Charlie has an NFT for a loan in his address, the DApp can deliberately choose UTxOs so that the UTxO with that NFT is consumed. Since the NFT can still be re-output to Charlie's address, Charlie may not even notice if the malicious DApp makes a change to his loan's settings. AFAIK even hardware wallets do not ask for the confirmation of any datums when signing a transaction.

I'm not sure of the best solution to this. One method would be to allow users to blacklist NFTs but I do not know how this can be trustlessly enforced. It may be that the ultimate solution is not based on the coin selection algorithm, but being that this is a CPS, I think it is worth mentioning this risk to DeFi. Whatever CIPs look to address this CPS should also mention how they are addressing this security risk or why they are choosing not to address it directly.

Edit: After thinking about this a bit more, the issue is mostly when the NFT is round-tripped to the same address (an actual value transfer should be noticed by the user). I think I can just disallow this scenario on the DApp side. It is basically operating on the assumption that coin selection algorithms cannot be trusted. I'm not sure if it is still worth mentioning in this CPS.

@rphair
Copy link
Collaborator

rphair commented Dec 13, 2023

@fallen-icarus I would agree with your hindsight that this problem can only be properly addressed at the dApp level, but also wonder why (and if) wallets don't have a user preference selectable for "Leave my NFTs [if not all native tokens] at the UTxOs where they currently reside unless they're being sent outside my wallet" which could be set for wallets sensitive to this scenario.

If the use of NFTs for ID / tagging in general, or Beacon Tokens specifically, becomes wider I think the community would welcome a CPS about this particular issue, and then in turn could reference this CPS and how current coin selection algorithms might be exploited. cc @SIDANWhatever

@rphair
Copy link
Collaborator

rphair commented Dec 13, 2023

@Ryun1 do you feel you have enough of an answer to these two questions (#611 (comment), #611 (comment)) unanswered in the dialogue at this time? I've approved it at an earlier stage but won't merge it until you confirm & I'll give @Crypto2099 a change to acknowledge the last round of commits as well.

@twwu123
Copy link
Contributor

twwu123 commented Dec 14, 2023

After mulling over this CPS for a bit, there is also a small missing problem statement. CIP-2 also doesn't care about minUTxOValues in change outputs, because if there isn't enough, you can just increase the fees paid to balance the transaction. Which doesn't work at all when tokens are involved. I'll add this to this CPS for consideration also.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Thanks @twwu123, that's an excellent addition and this still looks acceptable to me: just confirming again after all the changes in the meantime since my first review.

p.s. @Ryun1 the 2 questions from #611 (comment) have both been resolved now.

@rphair
Copy link
Collaborator

rphair commented Jan 18, 2024

@Crypto2099 it looks like your #611 (comment) has been taken care of... do you have any reservations before we merge this?

@rphair
Copy link
Collaborator

rphair commented Mar 16, 2024

Merging since discussion already concluded 2 months ago with no reservations & we now have another potential solution to this CPS (#785) so need to have this document in the repository baseline.

@rphair rphair merged commit 920d529 into cardano-foundation:master Mar 16, 2024
@rphair rphair removed the State: Last Check Review favourable with disputes resolved; staged for merging. label Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Wallets Proposals belonging to the 'Wallets' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants