Skip to content

add calc_bonds_given_shares_and_rate to rust utils#624

Merged
dpaiton merged 4 commits intohyperdrive_v0.0.16from
dpaiton/add-calc-bonds
Oct 23, 2023
Merged

add calc_bonds_given_shares_and_rate to rust utils#624
dpaiton merged 4 commits intohyperdrive_v0.0.16from
dpaiton/add-calc-bonds

Conversation

@dpaiton
Copy link
Copy Markdown
Contributor

@dpaiton dpaiton commented Oct 19, 2023

Adds a Rust version of the solidity calculateInitialBondReserves function in HyperdriveMath.sol.

edit: The below has been resolved with the most recent commit; there was a bug in the calculation.


I have a draft here for wrapping this in Python delvtech/hyperdrive-bindings#16
It can't run in CI until we merge this PR and cut a new hyperdrive release. However, I can run the tests locally.

I am finding that the calculate_bonds_given_shares_and_rate test in pyperdrive is giving me an interesting error:

FAILED system_tests/wrapper_tests.py::test_calculate_bonds_given_shares_and_rate - pyo3_runtime.PanicException: invalid exponent

I think this error is popping up in the underlying rust implementation from this PR, but I don't know why.

Here's are the settings for the test:

sample_pool_config = PoolConfig(
    baseToken="0x1234567890abcdef1234567890abcdef12345678",
    initialSharePrice=str(1 * 10**18),  # 1e18
    minimumShareReserves=str(1 * 10**17),  # 0.1e18
    minimumTransactionAmount=str(1 * 10**16),  # 0.001e18
    positionDuration=str(604_800),
    checkpointDuration=str(86_400),
    timeStretch=str(1 * 10**17),  # 0.1e18
    governance="0xabcdef1234567890abcdef1234567890abcdef12",
    feeCollector="0xfedcba0987654321fedcba0987654321fedcba09",
    Fees=Fees(curve=str(0), flat=str(0), governance=str(0)),
    oracleSize=str(10),
    updateGap=str(3_600),
)


sample_pool_info = PoolInfo(
    shareReserves=str(1_000_000 * 10**18),
    shareAdjustment=str(0),
    bondReserves=str(2_000_000 * 10**18),
    lpTotalSupply=str(3_000_000 * 10**18),
    sharePrice=str(1 * 10**18),
    longsOutstanding=str(0),
    longAverageMaturityTime=str(0),
    shortsOutstanding=str(0),
    shortAverageMaturityTime=str(0),
    withdrawalSharesReadyToWithdraw=str(0),
    withdrawalSharesProceeds=str(0),
    lpSharePrice=str(1 * 10**18),
    longExposure=str(0),
)

Following the equation for bonds:
mu * (z - zeta) * (1 + apr * t) ** (1/tau)

I would expect the output to be

y = 1 * (100000 - 0) * (1 + 3.742473403678144 * 0.02 ) ** (1/0.026677875377436178)
y = 1496383.181139349

Not sure why this would be invalid.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 19, 2023

Hyperdrive Gas Benchmark

Benchmark suite Current: 1852d27 Previous: 5dae7f5 Deviation Status
addLiquidity: min 733 gas 733 gas 0% 🟰
addLiquidity: avg 55905 gas 55943 gas -0.0679%
addLiquidity: max 97572 gas 97572 gas 0% 🟰
checkpoint: min 536 gas 536 gas 0% 🟰
checkpoint: avg 30350 gas 30347 gas 0.0099% 🚨
checkpoint: max 79983 gas 79983 gas 0% 🟰
closeLong: min 733 gas 733 gas 0% 🟰
closeLong: avg 23287 gas 23256 gas 0.1333% 🚨
closeLong: max 106163 gas 105226 gas 0.8905% 🚨
closeShort: min 802 gas 802 gas 0% 🟰
closeShort: avg 24733 gas 24735 gas -0.0081%
closeShort: max 105686 gas 100827 gas 4.8191% 🚨
initialize: min 662 gas 662 gas 0% 🟰
initialize: avg 160854 gas 160854 gas 0% 🟰
initialize: max 234375 gas 234375 gas 0% 🟰
openLong: min 735 gas 735 gas 0% 🟰
openLong: avg 55808 gas 55905 gas -0.1735%
openLong: max 225577 gas 225577 gas 0% 🟰
openShort: min 690 gas 690 gas 0% 🟰
openShort: avg 56481 gas 56403 gas 0.1383% 🚨
openShort: max 220946 gas 220946 gas 0% 🟰
removeLiquidity: min 755 gas 755 gas 0% 🟰
removeLiquidity: avg 79550 gas 79550 gas 0% 🟰
removeLiquidity: max 184551 gas 184551 gas 0% 🟰

This comment was automatically generated by workflow using github-action-benchmark.

@dpaiton dpaiton marked this pull request as ready for review October 19, 2023 22:09
@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Oct 19, 2023

Coverage Status

coverage: 97.117%. remained the same when pulling 1852d27 on dpaiton/add-calc-bonds into 5dae7f5 on main.

@wakamex
Copy link
Copy Markdown
Contributor

wakamex commented Oct 23, 2023

can we get rid of tau in the docstring? I believe the original definition for that was normalized_time / time_stretch but in this case since we assume normalized_time is 1, it just becomes time_stretch

@wakamex
Copy link
Copy Markdown
Contributor

wakamex commented Oct 23, 2023

maybe clarify that apr is actually the target rate. I prefer target_rate to apr since apr is just a type of rate, and we use fixed_rate and variable_rate elsewhere.

@wakamex
Copy link
Copy Markdown
Contributor

wakamex commented Oct 23, 2023

I would follow the style of the docstrings we already reviewed and merged into elf-sims. I'm assuming we didn't make any incorrect calls then 😉
delvtech/agent0@94f363e#diff-b313c8dbfc991949469eb56e9a6e328e49c8f75b570e57e791f4e0e0c53196ecR371-R372

@wakamex
Copy link
Copy Markdown
Contributor

wakamex commented Oct 23, 2023

@dpaiton dpaiton changed the base branch from main to hyperdrive_v0.0.16 October 23, 2023 20:34
@dpaiton dpaiton force-pushed the dpaiton/add-calc-bonds branch from 1852d27 to 24052e5 Compare October 23, 2023 20:40
@dpaiton
Copy link
Copy Markdown
Contributor Author

dpaiton commented Oct 23, 2023

can we get rid of tau in the docstring? I believe the original definition for that was normalized_time / time_stretch but in this case since we assume normalized_time is 1, it just becomes time_stretch

The docs are admittedly confusing, but I don't think we should make that change in this PR. The rust docs should be the same or a superset of the solidity docs. So I recommend making your own PR for this change, and make it in both the SOL & RS implementations.

@dpaiton
Copy link
Copy Markdown
Contributor Author

dpaiton commented Oct 23, 2023

maybe clarify that apr is actually the target rate. I prefer target_rate to apr since apr is just a type of rate, and we use fixed_rate and variable_rate elsewhere.

the variable name is chosen to match the solidity code. If you'd like to change the variable name, you should submit a PR that changes it in both places after this one lands.

@dpaiton
Copy link
Copy Markdown
Contributor Author

dpaiton commented Oct 23, 2023

I would follow the style of the docstrings we already reviewed and merged into elf-sims. I'm assuming we didn't make any incorrect calls then 😉 delvtech/elf-simulations@94f363e#diff-b313c8dbfc991949469eb56e9a6e328e49c8f75b570e57e791f4e0e0c53196ecR371-R372

same annoying reply as above unfortunately -- docstring fixes should be their own PR, as these ones are a mirror of what is in solidity.

@dpaiton
Copy link
Copy Markdown
Contributor Author

dpaiton commented Oct 23, 2023

Comment thread crates/hyperdrive-math/src/utils.rs Outdated
Comment thread crates/hyperdrive-math/tests/integration_tests.rs
Comment thread crates/hyperdrive-math/tests/integration_tests.rs Outdated
Comment thread crates/hyperdrive-math/tests/integration_tests.rs
Copy link
Copy Markdown
Contributor

@jalextowle jalextowle left a comment

Choose a reason for hiding this comment

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

Approved with some nits. Our pipeline didn't pick this up since this isn't based on main, so I can test it locally before cutting the release. We'll need to make sure to add this commit to another PR targeting main to get it into v0.0.17.

@dpaiton dpaiton merged commit b81d4db into hyperdrive_v0.0.16 Oct 23, 2023
@dpaiton dpaiton deleted the dpaiton/add-calc-bonds branch October 23, 2023 22:32
@dpaiton dpaiton restored the dpaiton/add-calc-bonds branch October 23, 2023 22:32
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.

4 participants