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

Switching to generic Amount branch of MASP crate. #1779

Closed
wants to merge 1 commit into from

Conversation

murisi
Copy link
Contributor

@murisi murisi commented Jul 31, 2023

Describe your changes

Switched the MASP crate branch used by Namada to anoma/masp#52 . The specific changes are as follows:

  • Changed *.toml files to point to new MASP crate commit hash
  • AllowedConversions are now represented by I32Sums
  • Fixed broken scripts/generator.sh script to facilitate testing of changes

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

Based on v0.20.1 .

Checklist before merging to draft

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

@murisi murisi force-pushed the murisi/masp-generic-amounts branch 2 times, most recently from 4529bbc to a7e80f6 Compare August 2, 2023 13:47
@murisi murisi requested review from batconjurer, a user and juped August 2, 2023 14:12
@murisi murisi marked this pull request as ready for review August 2, 2023 14:13
Copy link
Member

@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.

So I gave up putting individual comments on every instance, but from looking at it, I think it is possible to avoid all mentions of the ValueSum type. Using the explicit type alias will prevent using the two different type names for the same object and hopefully lead to clearer code.

The other nit was about the FromNt name. Other than these naming things, looks good to me.

apps/src/lib/client/rpc.rs Outdated Show resolved Hide resolved
apps/src/lib/client/tx.rs Outdated Show resolved Hide resolved
@@ -658,7 +658,7 @@ pub fn tokens() -> HashMap<Address, (&'static str, Denomination)> {
/// Temporary helper for testing, a hash map of tokens addresses with their
/// MASP XAN incentive schedules. If the reward is (a, b) then a rewarded tokens
/// are dispensed for every b possessed tokens.
pub fn masp_rewards() -> HashMap<Address, (u64, u64)> {
pub fn masp_rewards() -> HashMap<Address, (u32, u32)> {
Copy link
Member

Choose a reason for hiding this comment

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

Was this a necessary change? Neither type can be safely cast to i32. Of course, we know all these values fit into i32.

shared/src/ledger/masp.rs Outdated Show resolved Hide resolved
shared/src/ledger/masp.rs Outdated Show resolved Hide resolved
shared/src/ledger/masp.rs Outdated Show resolved Hide resolved
shared/src/ledger/masp.rs Outdated Show resolved Hide resolved
@murisi murisi force-pushed the murisi/masp-generic-amounts branch from a7e80f6 to 6957c5d Compare August 7, 2023 10:40
@Fraccaman Fraccaman closed this Sep 6, 2023
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