Skip to content

Conversation

@murisi
Copy link
Collaborator

@murisi murisi commented Apr 14, 2025

Describe your changes

Wrote a Cargo example that derives constraints on shielded reward parameters in order to ensure that end-users get non-zero rewards. Essentially this example takes the native token supply, incentivisation threshold, target locked amount, and locked amount tolerance and computes bounds on inflation, token precision, and nominal proportional gain sufficient to yield non-zero rewards. See the formulas upon which this example program is based at: https://hackmd.io/oqLHjyvwRMaxSD5JJNrB2w#Setting-parameters-to-ensure-non-zero-rewards . This program can be run with the following command: cargo run --example shielded-rewards.

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

@murisi murisi force-pushed the murisi/shielded-rewards-example branch from 08cd41d to 471e5fd Compare April 14, 2025 15:41
@murisi murisi requested a review from brentstone April 14, 2025 15:49
@github-actions github-actions bot added the backport-libs-0.150 Backport to libraries 0.150 maintenance branch label Apr 14, 2025
@murisi murisi force-pushed the murisi/shielded-rewards-example branch from 471e5fd to 0559439 Compare April 14, 2025 16:12
@murisi murisi force-pushed the murisi/shielded-rewards-example branch from 0559439 to 848bd44 Compare April 14, 2025 16:14
@murisi murisi requested a review from cwgoes April 14, 2025 16:15
@murisi murisi force-pushed the murisi/shielded-rewards-example branch from 848bd44 to 01593a9 Compare April 14, 2025 16:19
@murisi murisi added the MASP label Apr 15, 2025
@codecov
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (f2fb4c7) to head (0b3ae12).
Report is 137 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #4573   +/-   ##
============================
============================

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

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

Some nits in the doc strings. Also, I know that this is just an example, but it might actually end up being fairly useful. It will be annoying to have to restart the entire flow because of malformed input. I would suggest two UX improvements: One allow parsing all inputs from a file optionally (this is once users have a rough idea of the inputs and just want to tweak things) and also, instead of eagerly erroring out on bad input, just loop until correct input is given.

precision, incent_code
);

// It must be that S*C/Y >= I, which implies that the reward rate
Copy link
Collaborator

Choose a reason for hiding this comment

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

C is defined below. Consider moving doc string up. Also, S seems to be missing a definition in the doc strings entirely.

Copy link
Collaborator Author

@murisi murisi Apr 15, 2025

Choose a reason for hiding this comment

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

@murisi murisi force-pushed the murisi/shielded-rewards-example branch from 16ff912 to 1cb9fcc Compare April 15, 2025 09:54
@github-actions github-actions bot added the backport-libs-0.251 Backport libraries to 0.251 maintenance branch label Apr 15, 2025
@tzemanovic tzemanovic removed the backport-libs-0.150 Backport to libraries 0.150 maintenance branch label Apr 15, 2025
@murisi murisi requested a review from Fraccaman April 15, 2025 17:17
@batconjurer batconjurer self-requested a review May 7, 2025 08:03
@brentstone
Copy link
Collaborator

@murisi we should merge this if we can, is it ok?

@murisi
Copy link
Collaborator Author

murisi commented May 16, 2025

@murisi we should merge this if we can, is it ok?

@brentstone This PR touches a file outside the examples directory in order to have greater control over decimal arithmetic, namely crates/core/src/token.rs. Even if the mathematics and computations in the examples are assumed to be correct (because they have been used successfully in the upgrades), we should try to get at least one approval to the changes in token.rs because an error there could be very problematic.

@brentstone
Copy link
Collaborator

@tzemanovic or @batconjurer would you mind reviewing?

@batconjurer
Copy link
Collaborator

This is the third time I've been asked to review this with my comments from the first review not being addressed. I'll say it again:
Also, I know that this is just an example, but it might actually end up being fairly useful. It will be annoying to have to restart the entire flow because of malformed input. I would suggest two UX improvements: One allow parsing all inputs from a file optionally (this is once users have a rough idea of the inputs and just want to tweak things) and also, instead of eagerly erroring out on bad input, just loop until correct input is given.

@murisi
Copy link
Collaborator Author

murisi commented May 20, 2025

This is the third time I've been asked to review this with my comments from the first review not being addressed. I'll say it again: Also, I know that this is just an example, but it might actually end up being fairly useful. It will be annoying to have to restart the entire flow because of malformed input. I would suggest two UX improvements: One allow parsing all inputs from a file optionally (this is once users have a rough idea of the inputs and just want to tweak things) and also, instead of eagerly erroring out on bad input, just loop until correct input is given.

I really don't know how I missed that comment. My bad. Both ideas (file input and error recovery) are very good. The former is a little awkward to implement because the input range of certain variables depends on the value selected for earlier variables, but I think this can be achieved. Indeed file inputs (and outputs) would also allow us to keep (and share) a record of how the precision parameters were derived. And the latter idea would go with increased input validation (like making sure that numbers are within certain ranges). Thanks for bringing up these points - will work on them today or tomorrow.

@murisi murisi force-pushed the murisi/shielded-rewards-example branch from 7609762 to 5bf8e92 Compare June 11, 2025 12:37
@murisi murisi force-pushed the murisi/shielded-rewards-example branch from 5bf8e92 to 0b3ae12 Compare June 11, 2025 12:40
@murisi murisi added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Jun 11, 2025
mergify bot added a commit that referenced this pull request Jun 11, 2025
@mergify mergify bot merged commit 5fa5fe1 into main Jun 11, 2025
26 of 28 checks passed
@mergify mergify bot deleted the murisi/shielded-rewards-example branch June 11, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-libs-0.251 Backport libraries to 0.251 maintenance branch MASP merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants