Skip to content
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

fix bug on bounty update #730

Merged
merged 3 commits into from
Aug 30, 2023
Merged

Conversation

taitruong
Copy link
Contributor

Bug in contract:

  • bounty with e.g. 100 Juno
  • owner updates to 50 Juno
  • owner gets 50 Juno returned
  • if owner accidentally sends 50 Juno on update, then 100 Juno needs to be send back - not 50

@taitruong taitruong mentioned this pull request Aug 19, 2023
@@ -259,7 +259,9 @@ pub fn update(
}
Ordering::Less => {
// If new amount is less, pay out difference to owner
let diff = old_amount.amount - new_amount.amount;
// in case owner accidentally sent funds, send back as well
let funds_send = may_pay(&info, &bounty.amount.denom)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix for the bug - also check test below.

.update(
1,
coin(300, "ujuno"),
// case: update bounty with lower amount + owner accidentally send funds
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test for the fix

)
.unwrap();
// - assert bounty
Copy link
Contributor Author

Choose a reason for hiding this comment

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

additional check on Bounty state for making sure all is fine - especially checking in tests for update and payout below.

@taitruong
Copy link
Contributor Author

Question is whether for Close, PayOut and UpdateOwnership funds should also be checked and send back.

Copy link
Member

@JakeHartnell JakeHartnell left a comment

Choose a reason for hiding this comment

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

Le nice!

@JakeHartnell JakeHartnell merged commit 51befb9 into DA0-DA0:bounties Aug 30, 2023
2 of 3 checks passed
JakeHartnell pushed a commit that referenced this pull request Nov 9, 2023
* check Bounty is correct in all tests

* fix update bounty with lower amount and owner accidentally send funds

* cleanup and test update with sending wrong denom
JakeHartnell pushed a commit that referenced this pull request Dec 29, 2023
* check Bounty is correct in all tests

* fix update bounty with lower amount and owner accidentally send funds

* cleanup and test update with sending wrong denom
JakeHartnell pushed a commit that referenced this pull request Jan 8, 2024
* check Bounty is correct in all tests

* fix update bounty with lower amount and owner accidentally send funds

* cleanup and test update with sending wrong denom
JakeHartnell pushed a commit that referenced this pull request Mar 26, 2024
* check Bounty is correct in all tests

* fix update bounty with lower amount and owner accidentally send funds

* cleanup and test update with sending wrong denom
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants