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

refactor finalize block #2482

Merged
merged 7 commits into from
Feb 27, 2024
Merged

refactor finalize block #2482

merged 7 commits into from
Feb 27, 2024

Conversation

tzemanovic
Copy link
Member

@tzemanovic tzemanovic commented Jan 31, 2024

Describe your changes

Refactored token, gov and PoS sub-systems integration in finalize block. The token and PoS logic is moved into their respective crates. Gov proposal exec depends on the shell as well as IBC and PoS so this is kept in crates/apps/src/lib/node/ledger/shell/governance.rs for now.

Indicate on which release or other PRs this topic is based on

0.31.5

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@tzemanovic tzemanovic added the modularization Part of Namada's modularization effort label Jan 31, 2024
tzemanovic added a commit that referenced this pull request Jan 31, 2024
@tzemanovic tzemanovic marked this pull request as ready for review January 31, 2024 13:57
@tzemanovic tzemanovic mentioned this pull request Jan 31, 2024
2 tasks
@Fraccaman Fraccaman mentioned this pull request Feb 1, 2024
tzemanovic added a commit that referenced this pull request Feb 1, 2024
@tzemanovic tzemanovic force-pushed the tomas/refactor-finalize-block branch from 8d2eb2a to 91a3992 Compare February 1, 2024 17:09
@tzemanovic tzemanovic mentioned this pull request Feb 5, 2024
tzemanovic added a commit that referenced this pull request Feb 5, 2024
@tzemanovic tzemanovic force-pushed the tomas/refactor-finalize-block branch from 91a3992 to 3270bee Compare February 5, 2024 13:23
tzemanovic added a commit that referenced this pull request Feb 6, 2024
* tomas/refactor-testing-addrs:
  changelog: add #2507
  core: prevent from using addresses for testing in non-test code
  changelog: add #2506
  gov: replace namada_state dep with namada_storage
  changelog: add #2503
  update all core types usages
  core: flatten types mod
  update replay_protection usage
  core: factor out the ledger::replay_protection mod into a new crate
  core: refactor out ledger::eth_bridge mod
  changelog: add #2493
  benches: fix the shell to update conversions on new epochs
  move shielded params from core into shielded_token
  move inflation from core to trans_token
  replace namada_state usage with namada_storage in token crates
  changelog: add #2482
  refactor finalize_block PoS updates
  refactor finalize_block governance updates
  refactor finalize_block token updates
@brentstone brentstone mentioned this pull request Feb 8, 2024
This was referenced Feb 9, 2024
tzemanovic added a commit that referenced this pull request Feb 19, 2024
@tzemanovic tzemanovic force-pushed the tomas/refactor-finalize-block branch from 3270bee to fdafaa6 Compare February 19, 2024 14:51
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: Patch coverage is 74.35159% with 89 lines in your changes are missing coverage. Please review.

Project coverage is 53.39%. Comparing base (c733be2) to head (fdafaa6).
Report is 81 commits behind head on main.

❗ Current head fdafaa6 differs from pull request most recent head f338483. Consider uploading reports for the commit f338483 to get more accurate results

Files Patch % Lines
crates/core/src/event.rs 47.87% 49 Missing ⚠️
crates/proof_of_stake/src/lib.rs 78.69% 36 Missing ⚠️
...rates/apps/src/lib/node/ledger/shell/governance.rs 89.47% 2 Missing ⚠️
...s/apps/src/lib/node/ledger/shell/finalize_block.rs 94.73% 1 Missing ⚠️
crates/tx/src/lib.rs 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2482      +/-   ##
==========================================
+ Coverage   53.38%   53.39%   +0.01%     
==========================================
  Files         302      303       +1     
  Lines      103403   103433      +30     
==========================================
+ Hits        55198    55231      +33     
+ Misses      48205    48202       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grarco
grarco previously approved these changes Feb 21, 2024
Copy link
Collaborator

@grarco grarco left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

tzemanovic added a commit that referenced this pull request Feb 22, 2024
* tomas/refactor-finalize-block:
  changelog: add #2482
  refactor finalize_block PoS updates
  refactor finalize_block governance updates
  refactor finalize_block token updates
tzemanovic added a commit that referenced this pull request Feb 23, 2024
* tomas/refactor-finalize-block:
  changelog: add #2482
  refactor finalize_block PoS updates
  refactor finalize_block governance updates
  refactor finalize_block token updates
batconjurer
batconjurer previously approved these changes Feb 26, 2024
@brentstone
Copy link
Collaborator

Can we also move

for ibc_event in self.wl_storage.write_log_mut().take_ibc_events() {
let mut event = Event::from(ibc_event.clone());
// Add the height for IBC event query
let height = self.wl_storage.storage.get_last_block_height() + 1;
event["height"] = height.to_string();
response.events.push(event);
}
out of apply_inflation? I suspect it may have been incorrectly placed into this function, it can at least go right into finalize_block::finalize_block or some other function, but it has nothing to do with inflation.

@brentstone
Copy link
Collaborator

Would it also be safer to call governance::finalize_block before token::finalize_block in case we want governance to execute changes before MASP update_allowed_conversions is called ever?

@brentstone
Copy link
Collaborator

Also, perhaps we could move proof_of_stake::log_block_rewards to proof_of_stake::rewards::log_block_rewards and proof_of_stake::record_slashes_from_evidence to proof_of_stake::slashing::record_slashes_from_evidence.

@tzemanovic
Copy link
Member Author

Would it also be safer to call governance::finalize_block before token::finalize_block in case we want governance to execute changes before MASP update_allowed_conversions is called ever?

Yeah, good call

@tzemanovic
Copy link
Member Author

@yito88 it looks like the ibc events were in the wrong place and they would have only been applied on new epochs

@tzemanovic tzemanovic dismissed stale reviews from batconjurer and grarco via 55895ac February 27, 2024 11:37
@tzemanovic tzemanovic force-pushed the tomas/refactor-finalize-block branch from 93bdcff to f338483 Compare February 27, 2024 12:52
tzemanovic added a commit that referenced this pull request Feb 27, 2024
* tomas/refactor-token:
  changelog: add #2493
  benches: fix the shell to update conversions on new epochs
  move shielded params from core into shielded_token
  move inflation from core to trans_token
  replace namada_state usage with namada_storage in token crates
  changelog: add #2482
@brentstone brentstone self-requested a review February 27, 2024 16:53
@tzemanovic tzemanovic merged commit 76934b0 into main Feb 27, 2024
9 of 12 checks passed
@tzemanovic tzemanovic deleted the tomas/refactor-finalize-block branch February 27, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modularization Part of Namada's modularization effort non-breaking-change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants