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

Murisi/reverse conversion at source #4288

Closed
wants to merge 6 commits into from

Conversation

murisi
Copy link
Collaborator

@murisi murisi commented Jan 31, 2025

Describe your changes

Based on the bug fix in #4287. An alternative to #4290 that simplifies the exchanging an amount to some past epoch. If the current epoch is 10 and it is desired to convert an asset from epoch 2 to epoch 6 when only conversions to the latest epoch, 10, are available. Then the following process is now done:

  1. Query the conversion from epoch 2 to epoch 10, call it A
  2. Query the conversion from epoch 6 to epoch 10, call it B. (If we were converting to epoch 10, then B would equal 0 since that is the conversion from an epoch to itself.)
  3. Then return C=A-B. Because of the way the conversion tree is constructed, C is the conversion from epoch 2 to epoch 6 that would have been returned during epoch 6.

So more generally query_allowed_conversion now returns the result of a subtraction A-B. And by definition, the second operand of the subtraction is 0 when converting to the latest epoch, which ultimately means the result is simply A in that case. Additionally, compute_exchanged_amount has been simplified removing the forward and backward conversions.

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:

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 98.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.04%. Comparing base (1465f26) to head (2c580b7).
Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
crates/core/src/masp.rs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4288   +/-   ##
=======================================
  Coverage   74.03%   74.04%           
=======================================
  Files         345      345           
  Lines      110047   110027   -20     
=======================================
- Hits        81476    81468    -8     
+ Misses      28571    28559   -12     

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

@murisi murisi mentioned this pull request Jan 31, 2025
3 tasks
@murisi murisi requested review from batconjurer, grarco and sug0 February 3, 2025 10:19
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.

lgtm even though personally I prefer the approach of #4290

@murisi
Copy link
Collaborator Author

murisi commented Feb 3, 2025

lgtm even though personally I prefer the approach of #4290

Understood. Thanks for comparing the two PRs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants