Skip to content

[DO NOT MERGE] adds PoC model for vault share price valuation#6434

Open
Tapanito wants to merge 12 commits intotapanito/lending-fix-amendmentfrom
tapanito/poc-vault-valuation
Open

[DO NOT MERGE] adds PoC model for vault share price valuation#6434
Tapanito wants to merge 12 commits intotapanito/lending-fix-amendmentfrom
tapanito/poc-vault-valuation

Conversation

@Tapanito
Copy link
Copy Markdown
Collaborator

Spec: XRPLF/XRPL-Standards#485

High Level Overview of Change

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.9%. Comparing base (7222150) to head (dbf45ae).

Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                       @@
##           tapanito/lending-fix-amendment   #6434   +/-   ##
==============================================================
  Coverage                            79.9%   79.9%           
==============================================================
  Files                                 878     861   -17     
  Lines                               67970   67941   -29     
  Branches                             7551    7548    -3     
==============================================================
- Hits                                54281   54258   -23     
+ Misses                              13689   13683    -6     

see 54 files with indirect coverage changes

Impacted file tree graph

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

Copy link
Copy Markdown
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Partial review

Comment thread src/test/basics/VaultSharePricing_test.cpp
Copy link
Copy Markdown
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

In addition to the comments below, I noticed there are a lot of places where you have comments describing the current state of the vault. e.g.

// Vault: assetsTotal=1000, omega=50 (loan outstanding), iota=100, sharesTotal=1000

I'd like to see those have actual BEAST_EXPECT checks. Either added, or replacing the comments, up to your judgement.

Partial review. Note to self: left off at testPaperLossThenActualDefault()

Comment thread src/test/basics/VaultSharePricing_test.cpp
Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp
@ximinez ximinez self-requested a review March 4, 2026 22:12
Copy link
Copy Markdown
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

These changes are good, but I think you abstracted a little bit too much away. A lot of the tests are just verifying that, for example redeemAssets is using the same math as redeem. Doesn't mean the math is right.

What I think would be a good balance is to check the value of all the expected... variables. I'll include a couple of examples below.

Comment thread src/test/basics/VaultSharePricing_test.cpp Outdated
Comment thread src/test/basics/VaultSharePricing_test.cpp
Comment thread src/test/basics/VaultSharePricing_test.cpp
Comment thread src/test/basics/VaultSharePricing_test.cpp
Comment thread src/test/basics/VaultSharePricing_test.cpp
@Tapanito
Copy link
Copy Markdown
Collaborator Author

Thanks @ximinez . Yeah, I think you're right, it was too abstract. I restored some of the concreteness with explicit value assertions.

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.

2 participants