Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
MESH-1271: Implement Capital Distributions #716
Conversation
Centril
requested review from
adamdossa,
vkomenda,
maxsam4,
satyamakgec and
fmiguelgarcia
Centril
changed the title
|
There are some places in the corporate-actions/lib.rs where there is a typo for the |
| ensure!(!expired(expires_at, payment_at), Error::<T>::ExpiryBeforePayment); | ||
|
|
||
| // Ensure `now <= payment_at`. | ||
| ensure!(<Checkpoint<T>>::now_unix() <= payment_at, Error::<T>::NowAfterPayment); |
satyamakgec
Nov 5, 2020
Member
I think it should be < only
I think it should be < only
Centril
Nov 5, 2020
Author
Contributor
I believe now <= start is generally consistent with what we use elsewhere, e.g., record_date <= start.
Moreover, there's no harm with being inclusive re. current time rather than requiring a strict relation.
I believe now <= start is generally consistent with what we use elsewhere, e.g., record_date <= start.
Moreover, there's no harm with being inclusive re. current time rather than requiring a strict relation.
satyamakgec
Nov 5, 2020
Member
I think then you have to make the same change for the remove_distribution() pallet. Where we are not allowing to remove at an inclusive time. I think as per the definition of payment_at user should able to withdraw its funds when now == payment_at.
I think then you have to make the same change for the remove_distribution() pallet. Where we are not allowing to remove at an inclusive time. I think as per the definition of payment_at user should able to withdraw its funds when now == payment_at.
Centril
Nov 5, 2020
Author
Contributor
I don't think that follows. However, there's actually a bug (fixing) in ensure_active_distribution; should have been now >= dist.payment_at, not now > dist.payment_at, which is (part of) the definition of an active distribution. You should then only be able to remove a distribution if it isn't active yet, i.e. the complement of now >= dist.payment_at, i.e. !(now >= dist.payment_at) => now < dist.payment_at.
I don't think that follows. However, there's actually a bug (fixing) in ensure_active_distribution; should have been now >= dist.payment_at, not now > dist.payment_at, which is (part of) the definition of an active distribution. You should then only be able to remove a distribution if it isn't active yet, i.e. the complement of now >= dist.payment_at, i.e. !(now >= dist.payment_at) => now < dist.payment_at.
|
@Centril we are missing the test suite as well in this PR. |
Following up with tests in a follow-up to this and the ballots PR. |
Based on #705.
Tests will be added in a different follow-up PR.
Also resolves https://polymath.atlassian.net/browse/MESH-1336.