anza-xyz / agave

Web-Scale Blockchain for fast, secure, scalable, decentralized apps and marketplaces.
https://www.anza.xyz/
Apache License 2.0
382 stars 179 forks source link

Deprecate saturating_add_assign!() macro #502

Open steviez opened 6 months ago

steviez commented 6 months ago

Problem

The following convenience macro exists to perform add-assign with saturating math: https://github.com/anza-xyz/agave/blob/9076348ef40e28975796c2ff25460bf3b25d41e4/sdk/src/lib.rs#L168-L175

However, support for this is stable in Rust as of 1.74.0: https://doc.rust-lang.org/std/num/struct.Saturating.html#

Proposed Solution

Nagaprasadvr commented 6 months ago

fyi working on this

Nagaprasadvr commented 6 months ago

is this approach fine @steviez ?

image

steviez commented 6 months ago

is this approach fine @steviez ?

Hi @Nagaprasadvr - thank you for your interest in working on this. I think the code you included above would result in the values in self not actually getting updated (it looks like you're creating new variables instead of mutating the values in self). Something like https://github.com/anza-xyz/agave/pull/523 is how I envisioned it.

I'm going to get some feedback on my approach, and hopefully reach a decision with the direction we want to go. So, maybe hang tight until the PR I mentioned ships. Once we have a general approach we like, you can start knocking some more of these out if you're still interested

Nagaprasadvr commented 6 months ago

is this approach fine @steviez ?

Hi @Nagaprasadvr - thank you for your interest in working on this. I think the code you included above would result in the values in self not actually getting updated (it looks like you're creating new variables instead of mutating the values in self). Something like #523 is how I envisioned it.

I'm going to get some feedback on my approach, and hopefully reach a decision with the direction we want to go. So, maybe hang tight until the PR I mentioned ships. Once we have a general approach we like, you can start knocking some more of these out if you're still interested

sure you are right , i checked it with rust playground

Nagaprasadvr commented 6 months ago

tried this approach - image

Output - image

steviez commented 6 months ago

Once all instances are removed, mark the macro as deprecated; it is a public function in sdk so we should probably let it ride as deprecated for a bit instead of immediately ripping out

The point of this issue is to get rid of the custom macro now that the standard library has a mechanism for what we want built in

Nagaprasadvr commented 6 months ago

Once all instances are removed, mark the macro as deprecated; it is a public function in sdk so we should probably let it ride as deprecated for a bit instead of immediately ripping out

The point of this issue is to get rid of the custom macro now that the standard library has a mechanism for what we want built in

ohk makes sense then we have to change types to Saturating<T>