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

Add methods for checked additions/subtractions of Amounts #748

Closed
james-chf opened this issue Nov 7, 2022 · 3 comments · Fixed by #1078
Closed

Add methods for checked additions/subtractions of Amounts #748

james-chf opened this issue Nov 7, 2022 · 3 comments · Fixed by #1078
Assignees
Labels

Comments

@james-chf
Copy link
Contributor

Amount::spend and Amount::receive panic if there is an overflow/underflow. We want methods as well to do checked additions or subtractions safely, e.g. we may want a VP to reject a transaction in the normal way if it would overflow rather than panicking.

@sug0
Copy link
Contributor

sug0 commented Nov 7, 2022

slight note: the referred methods only panic in debug builds. in release builds, no panic will occur, which is quite bad, because we may accept/reject a VP that would have otherwise been rejected/accepted.

@cwgoes
Copy link
Contributor

cwgoes commented Jan 14, 2023

We should always be using checked arithmetic. @bengtlofgren is this maybe a quick change you could do?

@james-chf
Copy link
Contributor Author

NB: there is now already checked_sub, just need a checked_add method (+ some test coverage)

/// Checked subtraction
pub fn checked_sub(&self, amount: Amount) -> Option<Self> {
self.micro
.checked_sub(amount.micro)
.map(|result| Self { micro: result })
}

juped added a commit that referenced this issue Feb 7, 2023
Checked arithmetic was also brought in by other branches, but this one
is the only one with the tests.

* james/mainline/checked-amounts:
  [ci] wasm checksums update
  Add changelog
  Add tests for Amount checked arithmetic
  Add Amount::checked_add method
@github-project-automation github-project-automation bot moved this from Todo to Tested in Devnet in Namada-Old Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Tested in Devnet
Development

Successfully merging a pull request may close this issue.

4 participants