This repository was archived by the owner on Nov 15, 2023. It is now read-only.
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.
Pallets: Treasury
spend()
should usePay
trait #13607Pallets: Treasury
spend()
should usePay
trait #13607Changes from 5 commits
a027594
29194c3
11cd988
57aacbe
67b9f7a
2b20924
3602844
dd99d7c
5f52015
1925806
f94e20a
293552f
a88fb24
3aaed66
a33d4dc
414e8e2
2f60a1b
182c733
0916aff
fdcd66a
5673e79
c6f9c5a
bb372a2
a56897d
69cc489
e07ee86
304f536
e7ecdbf
e689272
3cd5760
d97886d
805d595
bbf83fa
8aa1c3c
aabfadf
592b81b
b295ea7
1e85a67
eb4b2fe
1fdccb7
310578d
22e43a1
d96d541
dc891cb
3bd3c8e
1018464
c04955d
b5b5d0e
65c71a0
62b0901
5750f0b
e26dee6
0739c29
ac27c84
87b905c
5bc0af9
1fca0e9
7d712e2
db02823
415561d
449d484
4741037
9cad108
b644329
8a9903b
f07e8b0
34c859b
9237568
1ecd8cc
c029123
83a2097
d79cab0
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.
legacy
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.
I kept it as ProposalIndex because of #13607 (comment)
As opposed to introducing a new type.
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.
Yeah, but why the alternative would be ProposalCount?
there are no proposals for new flow, but pending payments
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.
At the time when that comment was made, the count was used for the Proposals storage map. a027594#diff-21571fa193fececabaa733a8fea9e3c6478e1cdcb86f58e3f325418b2555e81aR237
I could introduce
PendingPaymentsCount
type, but the type going to be a u32 as well, just like ProposalIndex. But yeah, it's likely a good idea for clarity in the code. Though i'll call itPendingPaymentsIndex
for consistency.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.
there might be not enough weight capacity left for
spend_funds_local
.and spend period might be quite long, for polkadot it is 24 days.
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.
we should be timing out after some
timeout
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.
pallet_xcm does not timeout now. but I think it should be on both sides
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.
what do you propose?
With the current implementation, we would simply keep retrying the transaction each spend period until it succeeds, and that seems sufficient to me.
Also, the treasury pallet knows nothing about the XCM timeouts nor does it even know anything about XCM.
Eg lots of parachains will implement their treasuries using the
PayFromAccount
orPayFungibles
implementations which have no timeouts. So i'm not convinced about adding any timeout logic into the substrate treasury implementation, as that is too specific to XCMThere 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.
the treasury pallet does not need to know about XCM to have a timeout for some async status checks it does.
if for some reason it never gets some of the fail/success statuses, by this design it keeps checking the status forever.
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.
I added a concept of retries, so we retry a payment a given number of times.
About the timeouts, It would only be useful in the XCM context, and in the case of XCM, when it expires, the
check_payment
method would return a status unknown, which we now treat as a failure, so it would fail immediately at that point. I think with that workflow, there won't be any extra utility for the timeouts, and if that use-case comes up, then we can include it in a subsequent PR.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.
I think the payment statuses should be check with a different interval, not SpendPeriod. if we separate the pay job and the check status one, we will be able to have more accurate weights for these jobs.
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.
That makes sense. let me explore this more
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.
which means we retry endlessly, looks wrong to me.
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.
I think there's no harm. This gives time for governance to solve the reason for the failure. But what would you propose?
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.
what if its not recoverable fail, how governance will solve it?
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.
on_initialize_proposals is not relevant here