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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Support transfer public credits from self.signer #2139

Closed
snowtigersoft opened this issue Oct 29, 2023 · 9 comments 路 Fixed by #2402
Closed

[Feature] Support transfer public credits from self.signer #2139

snowtigersoft opened this issue Oct 29, 2023 · 9 comments 路 Fixed by #2402

Comments

@snowtigersoft
Copy link

馃殌 Feature

I propose adding two new methods to credits.aleo, tentatively named transfer_by_signer_public and transfer_by_signer_public_to_private. These new methods would be similar to the existing transfer_public and transfer_public_to_private, but the payer in these new methods should be changed from self.caller to self.signer.

Motivation

Currently, when transfer_public and transfer_public_to_private are invoked via a program, the cost is deducted from the program's account, not the actual initiator's (i.e., the signer's) account. This may not be the desired behavior in some scenarios.

For example, if a smart contract needs to make transfers on behalf of a user, the existing design deducts credits from the smart contract itself, rather than from the user (the signer). This could not only potentially deplete the smart contract's credits unexpectedly but may also introduce security and permission-related concerns.

By introducing these two new methods, we can ensure that the cost is correctly deducted from the signer's account, which is more logical and safer.

Implementation

  1. Define the two new methods, transfer_by_signer_public and transfer_by_signer_public_to_private, in the credits.aleo file.
  2. The logic for these methods should be similar to that of the existing transfer_public and transfer_public_to_private, but self.caller should be replaced with self.signer.
  3. Update the documentation and comments appropriately to reflect the existence and purpose of these new methods.
  4. Add unit tests to validate the correctness and expected behavior of the new methods.

Affected components could include:

  • The credits.aleo file
  • Relevant unit tests
  • Documentation

Are you willing to open a pull request?

Yes, I am willing to submit a pull request and engage in subsequent code reviews and testing.

@vicsn
Copy link
Contributor

vicsn commented Oct 30, 2023

Great idea. Instead of creating a new function, perhaps we can pass the payer (self or self.signer) in as address argument, or it could even just be a boolean flag.

@w3henry
Copy link

w3henry commented Nov 8, 2023

Great idea. Instead of creating a new function, perhaps we can pass the payer (self or self.signer) in as address argument, or it could even just be a boolean flag.

I think either calling transfer_by_signer_public or by adding a signer parameter is dangerous because the contract can transfer the user's assets without the user's knowledge or care.
This means that every time a user interacts with a program, he has to look carefully at the program code to make sure it doesn't steal his public aleo credits.

However, this feature is necessary to make aleo credits and other programs to be composable.
So, it might be better and safer to follow the erc20/arc20, i.e. to add the approve and transferFrom functions to the credits.aleo program.

@evanmarshall
Copy link
Collaborator

There's danger in either path but approvals is my preferred option. It should be somewhat safer. It would be very nice to include this in the default credits.aleo program as I see forcing the wrapping of tokens to be an anti-pattern that will only make it more difficult for DApps to support many tokens.

@snowtigersoft
Copy link
Author

The approve method also has potential issues. If a contract requests an unrestricted or very high amount of authorization, it can transfer the user's assets without their knowledge. This has happened many times in the Ethereum ecosystem. As for the use of transfer_by_signer_public, in essence, it's no different from transfer_private. Currently, a contract can transfer a user's private tokens using transfer_private, and users need to carefully review this to notice it. This is not fundamentally different from supporting transfer_by_signer_public.

@evanmarshall
Copy link
Collaborator

Yes, there's danger in either path. In any case, if you call a sign a dangerous contract either design will permit draining all of your funds.

It's really a question of how pooling assets should work. Approvals enable users to control their own funds without depositing them in a pool. This enables a user to interact with many DApps simultaneously with the same pool of funds where a transfer is effectively changing who controls the funds so the user can't use the same pool of funds in parallel with another DApp.

For example, we could create a program that acts as a trusted oracle about which DApps have been hacked. Users could give it an u64 max approval and it could monitor other approvals. If something gets hacked, it can automatically transfer away your funds to a safe place.

I think approvals should be the preferred solution but nothing is perfectly safe but I do believe approvals offer some interesting new types of user experiences.

Ultimately, I think we should head in the direction of Permit2: https://blog.uniswap.org/permit2-and-universal-router so we can get gasless approvals but just signing messages but that may be a little too ambitious right now.

@snowtigersoft
Copy link
Author

Yes, the approvals method is quite important and necessary. Regarding transfer_by_signer_public, what we actually need is something akin to Ethereum's method of carrying value during a call, so the wallet can also alert the user about the impending expenditure of funds.

@d0cd
Copy link
Contributor

d0cd commented Mar 22, 2024

@snowtigersoft it doesn't seem that transfer_public_to_private_as_signer is necessary once you have transfer_public_as_signer since you can accomplish the functionality with two calls. Do you agree?

@snowtigersoft
Copy link
Author

snowtigersoft commented Mar 22, 2024

@snowtigersoft it doesn't seem that transfer_public_to_private_as_signer is necessary once you have transfer_public_as_signer since you can accomplish the functionality with two calls. Do you agree?

I don't think it works like that. When you invoke transfer_public_as_signer, the ACs are transferred to the receiver's account. How does the program then convert these ACs into private? This can only be done by the receiver. It's not feasible within the same program call.

I understand what you're saying. transfer_public_to_private_as_signer can achieve the same result as transfer_public_as_signer + transfer_public_to_private, but the signers for these two methods are different. Therefore, it's not possible to execute them simultaneously in a single call unless the signer and receiver are the same account.

@d0cd
Copy link
Contributor

d0cd commented Mar 22, 2024

Here's a program that demonstrates the desired functionality

import credits.aleo;

program credits_wrapper.aleo;

function transfer_public_to_private:
    input r0 as address.private;
    input r1 as u64.public;
    call credits.aleo/transfer_public_as_signer credits_wrapper.aleo r1 into r2;
    call credits.aleo/transfer_public_to_private r0 r1 into r3 r4;
    async transfer_public_to_private r2 r4 into r5;
    output r3 as credits.aleo/credits.record;
    output r5 as credits_wrapper.aleo/transfer_public_to_private.future;

finalize transfer_public_to_private:
    input r0 as credits.aleo/transfer_public_as_signer.future;
    input r1 as credits.aleo/transfer_public_to_private.future;
    contains credits.aleo/account[credits_wrapper.aleo] into r2;
    assert.eq r2 false;
    await r0;
    get credits.aleo/account[credits_wrapper.aleo] into r3;
    assert.eq r3 r0[2u32];
    await r1;

An associated test can be found here

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 a pull request may close this issue.

5 participants