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

feat: Add nv22 skeleton #1929

Merged
merged 9 commits into from
Dec 5, 2023
Merged

Conversation

TippyFlitsUK
Copy link

Addition of Network Version 22 skeleton

Addition of Network Version 22 skeleton
@TippyFlitsUK TippyFlitsUK marked this pull request as ready for review November 22, 2023 17:08
@jennijuju
Copy link
Member

@Stebalien @fridrik01 do we need to add a nv22-dev feature in cargo.toml have a #[cfg(feature = "nv22-dev")] for creating new fvm?

Copy link
Contributor

@fridrik01 fridrik01 left a comment

Choose a reason for hiding this comment

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

Accepting as long as test failures are addressed before merging.

EDIT: Also added some additional comments since I reviewed this originally quite late

fvm/src/machine/default.rs Outdated Show resolved Hide resolved
@jennijuju
Copy link
Member

@Stebalien @fridrik01 do we need to add a nv22-dev feature in cargo.toml have a #[cfg(feature = "nv22-dev")] for creating new fvm?

Could you please advise on this? (I noticed we do this in previous ones, assuming it’s for avoiding unintended potential consensus breaking change?

@fridrik01
Copy link
Contributor

@Stebalien @fridrik01 do we need to add a nv22-dev feature in cargo.toml have a #[cfg(feature = "nv22-dev")] for creating new fvm?

Could you please advise on this? (I noticed we do this in previous ones, assuming it’s for avoiding unintended potential consensus breaking change?

I don't know if its strictly necessary but given that @Stebalien is out, then it would be the safe option here, so I would vote yes.

@Stebalien Stebalien changed the title [WIP] feat: Add nv22 skeleton feat: Add nv22 skeleton Nov 29, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2023

Codecov Report

Merging #1929 (c2ee6dd) into master (f631076) will increase coverage by 0.12%.
Report is 3 commits behind head on master.
The diff coverage is 83.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1929      +/-   ##
==========================================
+ Coverage   75.59%   75.71%   +0.12%     
==========================================
  Files         153      157       +4     
  Lines       15118    15477     +359     
==========================================
+ Hits        11428    11718     +290     
- Misses       3690     3759      +69     
Files Coverage Δ
fvm/src/machine/default.rs 71.87% <100.00%> (-0.66%) ⬇️
shared/src/version/mod.rs 7.69% <ø> (ø)
fvm/src/gas/price_list.rs 78.31% <0.00%> (-1.07%) ⬇️

... and 16 files with indirect coverage changes

shared/src/version/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

We also need to add nv22-dev as a feature in fvm/Cargo.toml (see here for example)

fvm/src/gas/price_list.rs Outdated Show resolved Hide resolved
Co-authored-by: Aayush Rajasekaran <arajasek94@gmail.com>
fvm/src/gas/price_list.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

One last nit, but LGTM with that.

fvm/src/gas/price_list.rs Outdated Show resolved Hide resolved
@arajasek arajasek enabled auto-merge (squash) December 5, 2023 15:23
@arajasek arajasek merged commit aa2aa06 into filecoin-project:master Dec 5, 2023
12 checks passed
arajasek added a commit that referenced this pull request Dec 6, 2023
* [WIP] feat: Add nv22 skeleton

Addition of Network Version 22 skeleton

* Update mod.rs

* Update default.rs

* Update mod.rs

* Update fvm/src/gas/price_list.rs

Co-authored-by: Aayush Rajasekaran <arajasek94@gmail.com>

* Update nv21-dev to nv22-dev

* Add nv22-dev to Cargo.toml

* Fix formatting issue

* Update fvm/src/gas/price_list.rs

---------

Co-authored-by: Aayush Rajasekaran <arajasek94@gmail.com>
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.

6 participants