search

filecoin-project / builtin-actors

The Filecoin built-in actors
Other
78 stars 74 forks source link

initial updates to integrate baseline bug fix #1557

Open kkarrancsu opened 1 week ago

jennijuju commented 1 week ago

note for the maintainers: this FIP has not yet been accepted, so the PR should live in an integration/feat until then

rvagg commented 1 week ago

@kkarrancsu btw running make check will get you past the linting problems that get reported in CI here before you push

kkarrancsu commented 1 week ago

@kkarrancsu btw running make check will get you past the linting problems that get reported in CI here before you push

Sounds good, I will do that before any further pushes.

kkarrancsu commented 2 days ago

Hi @jennijuju, @anorth - zooming out a little bit - we initially wanted to prototype this for the purpose of making the FIP complete to be considered by core-devs and community. Are we at that stage, or do we need to get this to the finish line, with unit tests, etc?

jennijuju commented 2 days ago

Hi @jennijuju, @anorth - zooming out a little bit - we initially wanted to prototype this for the purpose of making the FIP complete to be considered by core-devs and community. Are we at that stage, or do we need to get this to the finish line, with unit tests, etc?

I think this is good enough for actor spec. For the FIP, you also need to specify where the ramp start epoch is set and when (IIUC values will be set at upgrade migration - @lemmih could you please confirm?

anorth commented 1 day ago

I agree this is probably enough to inform a spec in the FIP now, though failing tests might suggest that something is missed.

kkarrancsu commented 1 day ago

I agree this is probably enough to inform a spec in the FIP now, though failing tests might suggest that something is missed.

The remaining error results from updating the return value to return the additional two elements needed to compute the pledge based on how far along we've progressed in the ramped activation

pub epochs_since_ramp_start: i64,
pub ramp_duration_epochs: u64,

I'm looking at the pre_commit_requires_commd_test function in integration_tests/src/tests/batch_onboarding_deals_test.rs, but it isn't immediately clear how to fix this test error. If someone can point me in the right direction here, I can resolve it.