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

TransferAllowDeath possibly fails on transaction validation when the balance is around ExistentialDeposit #4843

Open
2 tasks done
KiriosK opened this issue Jun 20, 2024 · 8 comments
Labels
I2-bug The node fails to follow expected behavior. I5-enhancement An additional feature request.

Comments

@KiriosK
Copy link

KiriosK commented Jun 20, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

During the transaction validation withdraw_fee method is called at some point. Not sure which implementation is used for DOT, so will leave both of them (Currency and Fungible Adapters)

In both cases KeepAlive or Preserve is passed, so is it correct to think that the transaction will be reverted on the validation stage without entering the real transfer execution with AllowDeath specified?

match F::withdraw(
who,
fee,
Precision::Exact,
frame_support::traits::tokens::Preservation::Preserve,
frame_support::traits::tokens::Fortitude::Polite,
) {

match C::withdraw(who, fee, withdraw_reason, ExistenceRequirement::KeepAlive) {
Ok(imbalance) => Ok(Some(imbalance)),
Err(_) => Err(InvalidTransaction::Payment.into()),
}

Steps to reproduce

  1. Have around 1 DOT at your account.
  2. Attempt tranferAllowDeath, with the fee, that will drop the balance below 1 DOT.
    E.g. Balance: 1.001, Amount: 0.99, Fee: 0.011
  3. Get the following error, even though the transfer should be possible with AllowDeath specified.
    Invalid Transaction: Inability to pay some fees (e.g. account balance too low)
@KiriosK KiriosK added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Jun 20, 2024
@bkchr
Copy link
Member

bkchr commented Jun 20, 2024

2. Attempt tranferAllowDeath, with the fee, that will drop the balance below 1 DOT.
E.g. Balance: 1.001, Amount: 0.99, Fee: 0.011

The problem is that the fee is deducted first and that already brings the account below the ED and then the KeepAlive requirement of the fee payment is already failing and that rejects the transaction. Generally we should support this, but it would require substantial changes.

@bkchr bkchr added I5-enhancement An additional feature request. and removed I10-unconfirmed Issue might be valid, but it's not yet known. labels Jun 20, 2024
@KiriosK
Copy link
Author

KiriosK commented Jun 21, 2024

@bkchr
Thank you for confirmation!
Some kind of verification for the CallType, in case it may kill the account seems reasonable for the transaction verification/fee deduction.

but it would require substantial changes.

Could you tell me approximately how much time/minor versions does it usually take for this kind of enhancements?

@bkchr
Copy link
Member

bkchr commented Jun 21, 2024

I personally have some ideas on how this could be solved, but right now nothing where I would say that is what we should implement. Thus, I generally can not give you any estimation. Or do you want to work on this?

@KiriosK
Copy link
Author

KiriosK commented Jun 24, 2024

Since I don't have really deep understanding of the architecture, that would be nice to hear the ideas of how it could be solved from you.
This fix is necessary for me, so I'd like to contribute and work on this one.

@bkchr
Copy link
Member

bkchr commented Jun 24, 2024

I think one solution could be that we increment the providers in the transaction payment extension before we do any fee withdrawal and then decrease the providers in post_dispatch to "let the account die".

@KiriosK
Copy link
Author

KiriosK commented Jun 24, 2024

Thank you for the explanation, I'll check around this implementation part and maybe discuss some alternatives if they seems reasonable

@bkchr
Copy link
Member

bkchr commented Jun 24, 2024

Okay! Maybe best to start with a test and see how that does.

@ggwpez
Copy link
Member

ggwpez commented Jun 26, 2024

Locking could also work, but its fairly similar. Passing it in through a signed extension is probably a bit too much of a breaking change and would be annoying to propagate into the runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. I5-enhancement An additional feature request.
Projects
Status: Backlog
Development

No branches or pull requests

3 participants