Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
CIP-0126? | Multi-Stake Delegation from a Single Account #628
base: master
Are you sure you want to change the base?
CIP-0126? | Multi-Stake Delegation from a Single Account #628
Changes from 2 commits
2744bb2
cdafdd4
b38ef16
89bff9a
ae06747
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Scanning historical transactions for a specific metadatum label is rather inefficient. For wallet apps, that's a minor problem since they will typically want to present the whole transaction history to the user anyway and they only deal with one or a small set of wallets. But for dApps that are confronted with ever new wallets every moment, it might well be prohibitive.
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.
Agree here, but what could be the alternative? Also for external applications that need this information, they could build any off-chain solution to make this efficient.
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.
To be honest, if this standard were to be adopted the only way I would support it as a dApp developer is if the wallet itself either:
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.
There's no such thing as user's wallet. There are utxos at addresses that are being consumed. Consider this: I construct an atomic swap transaction and put metadata that redefines the portfolio. I send it to the swap counterparty and they sign it. Who of us now has the delegation preferences updated?
Or even simpler, I am a malicious NFT minting dApp that wants to steal user's delegation. I put a metadata hash but do not disclose it to the user (which is normally expected)...
It is important to note that currently metadata posting never imposes any financial consequences for the users. But with this proposal adopted, it will.
This should not be accepted without a clear algorithm how to go from a chain of transactions to a mapping of
payment_address -> delegation_map
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.
Thanks for the feedback @klntsky, only updating the metadata doesn't affect the users delegation, the delegation can only be altered by submitting certificates and the right signatures in the transaction, so a malicious dApp cant steal user delegation by only updating the metadata of the transaction. The portfolio metadata is only intended to be consumed by off chain clients for context, I.E to determined the users preferences vs theirs actual stake distribution
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.
@AngelCastilloB
They do not specify the amount, so that any ADA that is on a stake pubkey is automatically delegated.
Which means a wallet or a dApp that consumes this info will find the wrongly updated metadata and prompt the user to either perform a rebalance, or will silently generate a change distribution such that ADA is moved to a SP the user didn't want to prefer more.
Right now as I understand there is no well-defined connection between user's "identity" and metadata, and what is this "identity" is also undecided.
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.
Ah I see your point now @klntsky, so yes it would be possible for a malicious dApp to move the percentages, but it wouldn't be possible to make the user delegate to a different pool.
So I guess the attack vector you are suggesting here would be for an adversarial pool to create a dApp that when it detects an user that is multi-delegating and one of the pools is multi-delegating to is in his control, it would then change the percentages to make it more favorable for him.
I think we have a few options here:
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.
Yep. That would also make it unambiguous who is the issuer of the metadata in case of multiple user inputs. Moreover, to support this edge case, user address or pubkeyhash can be used as a key in the map.
There is no way to prevent consuming an UTxO from another wallet
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 Random Improve really a good coin selection algorithm for this? Is it – in an unmodified form – even a good coin selection algorithm for wallets with native tokens?
Shouldn't it at least be modified to select UTxOs from all stakes as proportional as possible and not leave that to the randomness of the algorithm?
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.
Random Improve was just provided as an example. The point here would be that implementing multi-delegation shouldn't require changing the current input selection algorithm to accommodate it, the wallet could be using an input selection strategy that optimize for a given use case, ideally we dont want to disrupt that, as forcing the wallet to come up with a new input selection algorithm that satisfies his use case and multi delegation could be too problematic. Also during our internal tests, just directing the change output was sufficient to advert drift in the long run
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 merging this as written would ensure that the CIP stays in
Proposed
status forever. Depending on the outcome of the discussion planned in my review above, this could be a sensible approach... although if other implementations are on the horizon then a more flexible acceptance threshold should be defined here.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.
@AngelCastilloB - continued in #628 (comment) with a reason why it may actually make sense to leave this apparently unsatisfiable criterion on the list.