Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Fix treasury kept and spend when emptied#3880

Merged
gui1117 merged 12 commits into
masterfrom
gui-fix-treasury
Oct 24, 2019
Merged

Fix treasury kept and spend when emptied#3880
gui1117 merged 12 commits into
masterfrom
gui-fix-treasury

Conversation

@gui1117

@gui1117 gui1117 commented Oct 22, 2019

Copy link
Copy Markdown
Contributor

Fix #3879
Superseed #3797

Done:

  • treasury account is created at genesis config.
  • pot doesn't include existential deposit thus pot can be entirely spend with no issue.

TODO:

  • update polkadot: just includes Config in construct_runtime
  • fix some tests using runtime genesis config

Alternatives:

  • we could also keep old behavior with treasury being created when it happens that dilution is more than ED. but seem better that we create it once and for all.

Notes:

  • I still kept resolve_creating to ensure it still works on Kusama if there is no treasury account then it will just get created once on_dilution mint more than ED instead of never be created.
    Also in case someone forget to include Config in construct runtime (thus account is not created) then this account can be created later as well.

@gui1117 gui1117 added the A0-please_review Pull request needs code review. label Oct 22, 2019
@gavofyork gavofyork added the U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Oct 22, 2019
Comment thread srml/treasury/src/lib.rs Outdated
Comment thread srml/treasury/src/lib.rs Outdated
Comment thread srml/treasury/src/lib.rs
fn on_unbalanced(amount: NegativeImbalanceOf<T>) {
T::Currency::resolve_creating(&Self::account_id(), amount);
// Must resolve into existing but better be safe.
let _ = T::Currency::resolve_creating(&Self::account_id(), amount);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that we have better runtime logging I do get tempted to do something like resolve_exisiting().unwrap_or_else(|_| { log("CODE RED, SOS"); resolve_creating() ).

But I assume it depends on the proof. If it is logically impossible to cause account creation then this is also good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

interesting indeed a lot of such safe operation could be logged 🤔

Comment thread srml/treasury/src/lib.rs Outdated
gui1117 and others added 3 commits October 22, 2019 23:15
Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

@kianenigma kianenigma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PR looks good already;
Maybe we can add a test case to make sure:

  • Real pot (before subtracting ed) is always above ed even if more is requested to spend
  • Integration test fo the proper creation of treasury account similar to what will happen in KSM

@gui1117

gui1117 commented Oct 24, 2019

Copy link
Copy Markdown
Contributor Author

I would very suspect that treasury does exist already in kusama, but anyway it is fine. and I added both tests.

another solution would be to lazily create the account if it happens that it doesn't exist but whatever is OK

@kianenigma kianenigma added A7-looksgoodtestsfail and removed A0-please_review Pull request needs code review. labels Oct 24, 2019
@gui1117

gui1117 commented Oct 24, 2019

Copy link
Copy Markdown
Contributor Author

test are failing at least because of some CI issue, let's retry them

Comment thread srml/treasury/src/lib.rs

@kianenigma kianenigma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could use the suggestion for test otherwise LGTM and ready to merge.

Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@gui1117 gui1117 merged commit 8161896 into master Oct 24, 2019
@gui1117 gui1117 deleted the gui-fix-treasury branch October 24, 2019 15:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Treasury pot is both kept and spend when emptied.

4 participants