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 static lifetime to emission amounts calculation #4851

Merged

Conversation

hansieodendaal
Copy link
Contributor

Description

Added a static lifetime to emission_amounts calculation. This is needed if the pub fn emission_amounts will be called from an external client.

Motivation and Context

The &'[u64] return value was at odds with the emission_decay: &'static [u64] definition.

How Has This Been Tested?

Unit tests
External client

@stringhandler
Copy link
Collaborator

this doesn't seem right

@CjS77 CjS77 added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 25, 2022
@hansieodendaal
Copy link
Contributor Author

this doesn't seem right

The definition for self.emission_decay is pub(in crate::consensus) emission_decay: &'static [u64],, but &[u64] is returned for self.emission_decay.
Trying to call ConsensusConstants::emission_amounts from an external client in a separate function causes the return value to be dropped prematurely.

@stringhandler
Copy link
Collaborator

yeah I think the original struct is sus, but this method is fine (when you take the original struct into account)

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

utACK

Not really happy, but this PR is not the problem

@CjS77 CjS77 removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 25, 2022
@stringhandler stringhandler merged commit 5b0eb04 into tari-project:development Oct 25, 2022
@hansieodendaal hansieodendaal deleted the ho_static_lifetime branch October 25, 2022 10:37
@CjS77
Copy link
Collaborator

CjS77 commented Oct 25, 2022

Yeah, bad code smell

@CjS77
Copy link
Collaborator

CjS77 commented Oct 25, 2022

Oh, this is a reference to the decay parameters, which are in fact static

sdbondi added a commit to sdbondi/tari that referenced this pull request Nov 7, 2022
* development: (52 commits)
  chore: better help for seed-words command (tari-project#4885)
  fix(ci): resolve windows binary builds (tari-project#4883)
  fix(ci): correct ARM64 builds (tari-project#4876)
  fix(comms/peer_manager): fix possible panic in offline calc (tari-project#4877)
  feat!: impl final tari pow algorithm (tari-project#4862)
  fix(ci): selectively revert resolver for arm64 builds (tari-project#4871)
  chore(deps): bump actions/checkout from 2 to 3 (tari-project#4873)
  fix: delete orphans if they exist (tari-project#4868)
  chore: replace manual implementation of char methods (tari-project#4864)
  chore: fix potentially buggy split of string into lines (tari-project#4863)
  fix(ci): update GHA set-output plus dependabot schedule for GHA (tari-project#4857)
  fix(base-node): use less harsh emoji for unreachable node (tari-project#4855)
  fix(core): add txo version checks to async validator (tari-project#4852)
  feat: add static lifetime to emission amounts calculation (tari-project#4851)
  v0.38.8
  feat: add opcode versions (tari-project#4836)
  fix: remove clear_on_drop dependency (tari-project#4848)
  fix(base-node): use Network::from_str to parse network in cli (tari-project#4838)
  ci: remove circleci
  test: add cucumber critical (tari-project#4823)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants