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

Switch to strongly typed wrappers in VMLimitConfig #4139

Open
Longarithm opened this issue Mar 18, 2021 · 4 comments
Open

Switch to strongly typed wrappers in VMLimitConfig #4139

Longarithm opened this issue Mar 18, 2021 · 4 comments
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-housekeeping Category: Refactoring, cleanups, code quality P-low Priority: low

Comments

@Longarithm
Copy link
Member

Longarithm commented Mar 18, 2021

The discussion started here: #4107 (comment)

Short motivation:

  • explicit type names, like Bytesize, show exactly in which units quantities are scored
  • more convenient formatting
  • reduced probability of type confusion error

It can be applied to other structures, too.

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

CC: @matklad

@matklad
Copy link
Contributor

matklad commented Mar 19, 2021

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.

@bowenwang1996 bowenwang1996 added A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-housekeeping Category: Refactoring, cleanups, code quality labels Jun 28, 2021
@stale
Copy link

stale bot commented Sep 27, 2021

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 stale bot added the S-stale label Sep 27, 2021
@bowenwang1996 bowenwang1996 added P-low Priority: low and removed S-stale labels Sep 27, 2021
@stale
Copy link

stale bot commented Dec 27, 2021

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 stale bot added the S-stale label Dec 27, 2021
@stale
Copy link

stale bot commented Mar 29, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-housekeeping Category: Refactoring, cleanups, code quality P-low Priority: low
Projects
None yet
Development

No branches or pull requests

3 participants