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

Allow converting between Phoenix and Moonlight Dusk #2028

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

ureeves
Copy link
Member

@ureeves ureeves commented Jul 25, 2024

We add the convert function to the contract, leveraging the deposit and withdrawal capabilities to atomically swap Dusk between the Phoenix and Moonlight models.

The function is meant to be called directly - meaning as the first and only call - and takes a Withdraw as an argument, such that the user can prove ownership of either the account or address being deposited to.

Resolves: #1994

@ureeves ureeves requested a review from herr-seppia July 26, 2024 08:58
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

I must say it is a bit of a hack, leveraging the contract-withdrawal system in this way. But the more I think about it, the more it makes sense to me: Moonlight is the account model for the transfer-contract so in the end conversing from phoenix into moonlight and the other way round is nothing else than depositing and withdrawing into and from the transfer-contract itself.

One thing I don't understand though: Why does there always need to be a deposit present? A deposit only makes sense if we go from moonlight into phoenix edit: phoenix to moonlight. But when we go from phoenix to moonlight edit: moonlight to phoenix, there shouldn't be a deposit.

@ureeves
Copy link
Member Author

ureeves commented Jul 26, 2024

I must say it is a bit of a hack, leveraging the contract-withdrawal system in this way. But the more I think about it, the more it makes sense to me: Moonlight is the account model for the transfer-contract so in the end conversing from phoenix into moonlight and the other way round is nothing else than depositing and withdrawing into and from the transfer-contract itself.

This was exactly my line of thinking. Glad to see it resonates 😊.

One thing I don't understand though: Why does there always need to be a deposit present? A deposit only makes sense if we go from moonlight into phoenix. But when we go from phoenix to moonlight, there shouldn't be a deposit.

I have the feeling that this question comes from a place of placing one of the models at a different footing than the other.

The transactor must put forward the funds available - meaning have them available to be spent - for them to be able to securely withdraw in either model. If they don't we end up with an actual "mint", where the user would be able to generate Dusk out of thin air. Maybe I'm not understanding the question properly, so let me ask a question... Why shouldn't there be a deposit when coming from Phoenix?

On a more conceptual level... Deposits and withdrawals work in exactly the same way in Moonlight and Phoenix - i.e. they're symmetric operations under a model change. Therefore, it stands to reason that the same would apply for a combination of these two operations. In other words, an atomic combination of symmetric operations is itself symmetric.

miloszm
miloszm previously approved these changes Jul 26, 2024
Copy link
Contributor

@miloszm miloszm left a comment

Choose a reason for hiding this comment

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

I would be nice to have a wallet driven test and be able to show that wallet balance is the same before and after the operation, except for the gas spent. Nevertheless, I approve, looks really good to me.

@moCello
Copy link
Member

moCello commented Jul 26, 2024

One thing I don't understand though: Why does there always need to be a deposit present? A deposit only makes sense if we go from moonlight into phoenix edit: phoenix into moonlight. But when we go from phoenix to moonlight edit: moonlight into phoenix, there shouldn't be a deposit.

I have the feeling that this question comes from a place of placing one of the models at a different footing than the other.

The transactor must put forward the funds available - meaning have them available to be spent - for them to be able to securely withdraw in either model. If they don't we end up with an actual "mint", where the user would be able to generate Dusk out of thin air. Maybe I'm not understanding the question properly, so let me ask a question... Why shouldn't there be a deposit when coming from Phoenix?

On a more conceptual level... Deposits and withdrawals work in exactly the same way in Moonlight and Phoenix - i.e. they're symmetric operations under a model change. Therefore, it stands to reason that the same would apply for a combination of these two operations. In other words, an atomic combination of symmetric operations is itself symmetric.

Let me try to explain what I mean:

A deposit are funds that are being transferred from phoenix or moonlight dusk to a contract.
As such there basically shouldn't be any deposit set for the moonlight-phoenix conversion (one way or the other). This is not possible though in the phoenix to moonlight direction, because in that case the phoenix-circuit wouldn't check out (input-notes == output-notes + max_fee + deposit) which is why we are forced to set a deposit for phoenix to moonlight conversion.
In the moonlight to phoenix direction however, I don't understand why there is a deposit set.
After all we also don't set a deposit when withdrawing funds from a contract into either moonlight or phoenix dusk.

Addition after some more thoughts:
I do now understand. The convert function is deposit and withdraw in one: by setting the deposit for both moonlight and phoenix conversions, you make sure that the dusk are taken from one of the two models, and by ending with a withdrawal, you add the funds again to the respective other model.
It's quite 'hacky' and I have the feeling that there are some edge-cases that are not caught, but I cannot find any.

At the same time I also quite like the elegance of it.

@ureeves
Copy link
Member Author

ureeves commented Jul 26, 2024

A deposit are funds that are being transferred from phoenix or moonlight dusk to a contract. As such there basically shouldn't be any deposit set for the moonlight-phoenix conversion (one way or the other). This is not possible though in the phoenix to moonlight direction, ...

The problem I see here is that you're taking a conversion as there being no value deposited into the contract in the first place, but this is only true of the end state. What is happening is a deposit to and then a withdrawal from the same contract in an atomic way, so the usage of the deposit field is perfectly appropriate.

We add the `convert` function to the contract, leveraging the deposit
and withdrawal capabilities to atomically swap Dusk between the Phoenix
and Moonlight models.

The function is meant to be called directly - meaning as the first and
only call - and takes a `Withdraw` as an argument, such that the user
can prove ownership of either the account or address being deposited to.

Resolves: #1994
@ureeves ureeves force-pushed the swap-phoenix-moonlight branch from 2a1822b to ed860be Compare July 26, 2024 15:27
@ureeves ureeves requested a review from moCello July 29, 2024 07:21
Copy link
Member

@moCello moCello left a comment

Choose a reason for hiding this comment

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

LGTM

@ureeves ureeves merged commit 68bd0ac into master Jul 29, 2024
8 checks passed
@ureeves ureeves deleted the swap-phoenix-moonlight branch July 29, 2024 10:10
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.

Add mechanism to transfer Dusk between Moonlight and Phoenix
4 participants