-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refund logic #21
Refund logic #21
Conversation
# Conflicts: # contracts/PensionFundRelease.sol # test/PensionFundReleaseTest.js
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.
Please take a look at the comments
contracts/PensionFundRelease.sol
Outdated
// Confirm validators have refunded funds | ||
require(isBurnApproved()); | ||
// Confirm the next payment is due to be released | ||
require(isFundFreezePeriodEnded()); |
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 we can allow to burn even before the paymentTime
contracts/PensionFundRelease.sol
Outdated
refundedAmount = getPaymentAmount(); | ||
firtPaymentReleased = true; | ||
} else { | ||
refundedAmount = getPaymentAmount(); |
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 need to return all remaining amount, not next payment amount
contracts/PensionFundRelease.sol
Outdated
refundedAmount = balance(); | ||
} | ||
// Assumes intended interval is meant to recur regardless of claiming funds | ||
paymentTime = paymentTime + recurrentPaymentInterval; |
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 don't need to update paymentTime in burn case
test/PensionFundReleaseTest.js
Outdated
firstPaymentPercent, | ||
token.address, | ||
PAYOUT_PERCENT | ||
it("#14 should refund all roots and then refund balance of 0", async() => { |
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.
Please, don't forget to change tests after your changes in PR.
You need just to check that all ether amount are refunded, if burn was approved.
And also a negative scenario (burn was not approved -> failure)
# Conflicts: # migrations/2_deploy_IouRootsToken.js # migrations/3_deploy_PensionFundRelease.js # test/IouRootsTokenTest.js # test/PensionFundReleaseTest.js # truffle.js
relates to #21 |
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.
Good job, thanks!
No description provided.