near / nearcore

Reference client for NEAR Protocol
https://near.org
GNU General Public License v3.0
2.3k stars 602 forks source link

Switch to strongly typed wrappers in VMLimitConfig #4139

Open Longarithm opened 3 years ago

Longarithm commented 3 years ago

The discussion started here: https://github.com/near/nearcore/pull/4107#discussion_r595015826

Short motivation:

It can be applied to other structures, too.

Current blocker: bytesize::Bytesize doesn't support Hash trait.

CC: @matklad

matklad commented 3 years ago

More generally, I noticed that we use "bare" types a lot:

pub const CHUNK_REQUEST_RETRY_MS: u64 = 100;
pub type Gas = u64;

That's a bit surprising to me, as Rust generally has great facilities for strongly-typed primiteves.

I think such quantities fall into three buckets:

Timeouts: I believe we should use std::time::Duration for them always, as that's the standard type.

Byte Sizes: We've been using bytesize crate for those. The crate is good quality, but it needs some improvements, and the maintained is not super responsive, so it doesn't seem like a clear win. I wonder if we may want to just define our own ByteSize in near_primiteves_core? We might also ask someone (me?) to be a co-maintainer of the crate.

Domain Specific Types: Gas, Ballance and friends. Wrapping those into newtypes with arithmetics would require more work. On the other hand, as we care about arithmetic overflows, we might also get more leverage our of them by just not providing non-checked APIs?

Naturally, there's also the fact that all the code today doesn't use strong types, so migration would take some effort and could get stuck in the middle.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.