-
Notifications
You must be signed in to change notification settings - Fork 6
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
Merged
JuaniRios
merged 47 commits into
main
from
feature/plmc-518-comprehensive-community-round-tests
Apr 22, 2024
+1,315
−1,298
Merged
Changes from all commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
24373bf
upgrade worked
JuaniRios fcc5635
nit
JuaniRios 9c98105
Merge branch 'main' into politest-0.6.3
JuaniRios 9a6fe1a
format auction tests, remove v0-v1 funding migration due to old types…
JuaniRios 3377f17
save
JuaniRios 02466b7
changes
JuaniRios 951f67d
finish tests
JuaniRios 700fc4c
warnings
JuaniRios d0e9d91
Merge branch 'main' into feature/plmc-512-comprehensive-auction-round…
JuaniRios 9908d0c
Just's comments
JuaniRios da0328b
Just's comments
JuaniRios 5508ada
Just's comments
JuaniRios c909248
Just's comments.
JuaniRios b49e49d
fixed tests by changing logic
JuaniRios 64ea69c
min ticket now uses current bucket size
JuaniRios 1a88265
fix warnings
JuaniRios 00c638b
simplify test
JuaniRios 45a5fca
more bucket assets
JuaniRios 786d028
combine tests
JuaniRios d7889e5
remove unnecessary test
JuaniRios b3e0f97
Restructure community funding tests
JuaniRios db8522a
save
JuaniRios e091939
Add constant attribute and enhance test cases
JuaniRios 461c359
add lower bound of 100 USD to evaluations
JuaniRios cba3de3
move credential checks to fail mod
JuaniRios c82b3cc
:fire: remove evaluation over limit bench
JuaniRios 096df9f
:bug: fix benchmark
JuaniRios bd7f7ec
:children_crossing: rename extrinsic param to `ct_amount`
JuaniRios 94cefac
:white_check_mark: fix price calculation, add bidder contributor tests
JuaniRios 1fc98cd
:white_check_mark: insufficient funds test, failing outside community…
JuaniRios 8458ca2
:white_check_mark: all pallet tests passing, I'm happy with the paths…
JuaniRios e65245c
Merge branch 'main' into feature/plmc-518-comprehensive-community-rou…
JuaniRios 3fa45a2
:bug: fix integration tests
JuaniRios 82418b1
Merge branch 'main' into feature/plmc-518-comprehensive-community-rou…
JuaniRios 7406a56
Merge branch 'main' into feature/plmc-518-comprehensive-community-rou…
JuaniRios 5e87f5a
feedback
JuaniRios 82decef
feedback
JuaniRios f73acbb
save
JuaniRios e8e27f1
feedback
JuaniRios ae424ed
feedback
JuaniRios 3e0d2a6
last feedback addressing
JuaniRios 724db25
Merge remote-tracking branch 'origin/main' into feature/plmc-518-comp…
JuaniRios 428067d
merge
JuaniRios f2237a4
fmt + wap calculation change
JuaniRios 6487ace
fix warnings
JuaniRios 18fd306
remove test and split another
JuaniRios dea87cc
fix warnings, ignore test
JuaniRios File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
use super::*; | ||
|
||
#[macro_export] | ||
/// Example: | ||
/// ``` | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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