Skip to content

Conversation

@Fraccaman
Copy link
Collaborator

@Fraccaman Fraccaman commented Aug 22, 2023

Describe your changes

  • Completed Public Good Funding.
  • Fix an issue with signature verification.

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

0.21.1

Checklist before merging to draft

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

&funding.detail.target,
);
} else {
tracing::info!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we log this at a different level to make it stand out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, maybe warn is better

address,
);
} else {
tracing::info!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as before for the log level

for (address, percentage) in steward.reward_distribution {
let pgf_steward_reward = pgf_steward_reward
.checked_mul(&percentage)
.unwrap_or_default();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we log something in case of an overflow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this can never happen cause percentage is between 0 and 1

}: args::ResignSteward,
gas_payer: &common::PublicKey,
) -> Result<Tx, Error> {
// if !rpc::is_steward(client, &steward).await && !tx_args.force {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need these checks?

Copy link
Collaborator Author

@Fraccaman Fraccaman Aug 24, 2023

Choose a reason for hiding this comment

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

we need them, you are right!

@Fraccaman Fraccaman requested a review from grarco August 24, 2023 14:48
}
}

/// Get governane parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo

Suggested change
/// Get governane parameters
/// Get governance parameters

Comment on lines 449 to 450
let is_negative = (self.is_negative() || v.is_negative())
&& !(self.is_negative() && v.is_negative());
Copy link
Collaborator

@grarco grarco Aug 24, 2023

Choose a reason for hiding this comment

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

I think you can write this as

Suggested change
let is_negative = (self.is_negative() || v.is_negative())
&& !(self.is_negative() && v.is_negative());
let is_negative = self.is_negative() != v.is_negative();

grarco
grarco previously approved these changes Aug 24, 2023
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.

Minor comments but looks good to me, thanks!

Fraccaman added a commit that referenced this pull request Aug 28, 2023
* origin/fraccaman/pgf:
  pgf: minor fixes
  e2e: fix pgf test
  pgf: added vp pgf validation
  pgf: minor fixes, added basic tests
  pgf: added resign and update steward reward txs
  multisig: fix hashes ordering
  pgf: rework fundings storage
  pgf: improve storage api, fix tallying
  client: refactor router, fix minor ui issues
Fraccaman added a commit that referenced this pull request Aug 28, 2023
Fraccaman added a commit that referenced this pull request Sep 6, 2023
* origin/fraccaman/pgf:
  pgf: minor fixes
  e2e: fix pgf test
  pgf: added vp pgf validation
  pgf: minor fixes, added basic tests
  pgf: added resign and update steward reward txs
  multisig: fix hashes ordering
  pgf: rework fundings storage
  pgf: improve storage api, fix tallying
  client: refactor router, fix minor ui issues
Fraccaman added a commit that referenced this pull request Sep 6, 2023
* origin/fraccaman/pgf:
  pgf: minor fixes
  e2e: fix pgf test
  pgf: added vp pgf validation
  pgf: minor fixes, added basic tests
  pgf: added resign and update steward reward txs
  multisig: fix hashes ordering
  pgf: rework fundings storage
  pgf: improve storage api, fix tallying
  client: refactor router, fix minor ui issues
Fraccaman added a commit that referenced this pull request Sep 6, 2023
@Fraccaman Fraccaman merged commit 36fcbb4 into main Sep 6, 2023
@Fraccaman Fraccaman deleted the fraccaman/pgf branch September 6, 2023 21:03
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.

3 participants