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
✅ Comprehensive community round tests (518) #229
✅ Comprehensive community round tests (518) #229
Changes from 35 commits
24373bf
fcc5635
9c98105
9a6fe1a
3377f17
02466b7
951f67d
700fc4c
d0e9d91
9908d0c
da0328b
5508ada
c909248
b49e49d
64ea69c
1a88265
00c638b
45a5fca
786d028
d7889e5
b3e0f97
db8522a
e091939
461c359
cba3de3
c82b3cc
096df9f
bd7f7ec
94cefac
1fc98cd
8458ca2
e65245c
3fa45a2
82418b1
7406a56
5e87f5a
82decef
f73acbb
e8e27f1
ae424ed
3e0d2a6
724db25
428067d
f2237a4
6487ace
18fd306
dea87cc
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.
Why does a release of a hold fail? We are not actually sending tokens, right? Just releasing tokens that are already in the users account?
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.
As far as I understand from a quick test, its not possible to hold funds if it would put your free balance under ED. So I think this situation should in theory not happen. Lets say we have 100 tokens with ED = 1:
So balance should always contain at least ED free tokens in this case.
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.
This is the logic for placing a hold. It is called with
Protect
andForce
.The case for erroring out when there's no ED left after hold is when there is only 1 provider. And if I understand correctly, we could in the future introduce pallets that provide for the existence of the account, making it possible for it to live without an ED.
Therefore I prefer to be safe and do a check before releasing so we don't halt the whole project. I don't think most people will be getting a return below ED anyway, and if they do they shouldn't mind it not being returned.
@lrazovic any thoughts?
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.
For reference here is the logic for releasing an asset. It always checks the amount free + the released amount will be bigger than ED.
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.
From: paritytech/substrate#12951
So I don't think that
release
is infallible. Now I throw the question back at you: is this the best way to handle this possible failure? If yes, then we can resolve the comment.Tagging also @joepetrowski who maybe has some extra input on this.
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.
If it fails due to ED, we can cover it. If it fails for some other reason we probably don't want to continue the project transition because it's unexpected behavior and we should fix it and then force run the community start.
So I think we can resolve as is