-
Notifications
You must be signed in to change notification settings - Fork 0
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
docs: Expand readme for investments #33
base: main
Are you sure you want to change the base?
Conversation
Retrieve a vault by querying it from the Pool: | ||
|
||
```js | ||
const pool = await centrifuge.pool('1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should input here not be a number rather than string? Since pool IDs are uint64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should allow both. If we're parsing a URL like /pools/123/0xabc
to get the pool id and tranche id, it's easiest if the user doesn't have to do useTranche(Number(poolId), trancheId)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fair! Either way fine for now to keep as is
Invest in a vault: | ||
|
||
```js | ||
const result = await vault.increaseInvestOrder(1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be 1000*10**decimals
? I would make that clear by using the relevant type for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It currently accepts numbers, bigints and CurrencyBalances. If a number is passed, it does number*10**decimals
in the function. My thinking was that it would be good to not force users to work with the decimals and do conversions. But maybe it's confusing that increaseInvestOrder(1000)
and increaseInvestOrder(1_000_000_000n)
will invest the same amount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I understand the perspective. I like the focus on simplifying user input, but do worry here that it will rather create issues with incorrect inputs.
I would actually lean towards only allowing CurrencyBalance
as an input here. To force the user to clearly specify what the input denomination should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not ideal, but maybe that's the way to go if we want to avoid confusion.
|
||
```js | ||
const investment = await vault.investment('0x123...') | ||
// Will return an object containing: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really helpful 🙌
Co-authored-by: Jeroen <[email protected]>
Co-authored-by: Jeroen <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
+ Coverage 88.59% 89.25% +0.66%
==========================================
Files 24 24
Lines 3033 3034 +1
Branches 266 275 +9
==========================================
+ Hits 2687 2708 +21
+ Misses 339 319 -20
Partials 7 7 ☔ View full report in Codecov by Sentry. |
Description
This pull request...
#