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

[nv16] propagate gas outputs from fvm + integrate safer ffi #8593

Merged
merged 11 commits into from
May 10, 2022

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented May 4, 2022

@vyzo vyzo requested a review from raulk May 4, 2022 12:30
@vyzo vyzo requested a review from a team as a code owner May 4, 2022 12:30
@vyzo vyzo changed the base branch from master to feat/nv16 May 4, 2022 12:31
@vyzo vyzo force-pushed the fix/nv16-gas-outputs branch 2 times, most recently from 3b6be70 to a9b72d5 Compare May 4, 2022 12:47
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Tests that are failing are already failing.

@@ -869,89 +868,6 @@ func TestAddPiece512MPadded(t *testing.T) {
require.Equal(t, "baga6ea4seaqonenxyku4o7hr5xkzbqsceipf6xgli3on54beqbk6k246sbooobq", c.PieceCID.String())
}

func setupLogger(t *testing.T) *bytes.Buffer {
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why this needs to go entirely? I understand that generated no longer exists, but has all this functionality vanished from the library? Or is it just behind different APIs? If the latter, we should adapt the tests, not drop the coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its gone completely afaict.

Copy link
Member

@raulk raulk May 10, 2022

Choose a reason for hiding this comment

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

It isn't. AFAIK no functionality has been removed, only APIs have changed. Here's where the log is now:

https://github.com/filecoin-project/filecoin-ffi/blob/bb281b5dfcfb7f485056604c570a3c92f103cea9/cgo/util.go#L11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok feel free to reinstate, it is really ugly however.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that the form of the test is terrible, but it is providing certain coverage that we can't drop here.

Copy link
Member

Choose a reason for hiding this comment

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

Reinstated an adapted version here: 0d48654 (#8593)

@raulk raulk changed the title [nv16] propagate gas outputs from fvm [nv16] propagate gas outputs from fvm + integrate safer ffi May 10, 2022
@raulk raulk merged commit 1814187 into feat/nv16 May 10, 2022
@raulk raulk deleted the fix/nv16-gas-outputs branch May 10, 2022 14:15
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.

2 participants