Skip to content

changing APR to roundYield#46

Merged
jamiehewitt15 merged 6 commits intomainfrom
issue-45-roundYield
Mar 28, 2023
Merged

changing APR to roundYield#46
jamiehewitt15 merged 6 commits intomainfrom
issue-45-roundYield

Conversation

@jamiehewitt15
Copy link
Contributor

@jamiehewitt15 jamiehewitt15 commented Mar 24, 2023

Fixes #45

Changes proposed in this PR:

  • changing APR to roundyield
  • Adding proper APR = roundYield * 52
  • Lint fixes

@jamiehewitt15 jamiehewitt15 marked this pull request as ready for review March 24, 2023 11:17
@jamiehewitt15 jamiehewitt15 changed the title changing APR to roundyield changing APR to roundYield Mar 24, 2023
@idiom-bytes
Copy link
Member

Let's target this deployment for Monday.

Queryable fields:
```
chainID,nft_addr,did,symbol,name,ve_allocated ocean_allocated,ve_allocated_owner, ocean_allocated_owner,ve_allocated_realtime,ocean_allocated_realtime,ve_allocated_realtime_owner,ocean_allocated_realtime_owner,volume,is_purgatory,apr,apy,owner_addr,round
chainID,nft_addr,did,symbol,name,ve_allocated ocean_allocated,ve_allocated_owner, ocean_allocated_owner,ve_allocated_realtime,ocean_allocated_realtime,ve_allocated_realtime_owner,ocean_allocated_realtime_owner,volume,is_purgatory,apr,apy,roundYield,owner_addr,round
Copy link
Member

Choose a reason for hiding this comment

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

LGTM, saw we're still pushing apr out.

nftRewardsAmt / (nft.ocean_allocated + nft.ocean_allocated_owner)
const nftApy = wprToApy(nftRoundYield)
nftinfo[i].roundYield = nftRoundYield
nftinfo[i].apr = nftRoundYield * 52
Copy link
Member

Choose a reason for hiding this comment

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

I see we're now calculating and reporting both properly.

Copy link
Member

Choose a reason for hiding this comment

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

I also see that you just added roundYield rather than remove anything, so I'm going to reconcile this with df-web


expect(nftinfo[1].apy).toBeCloseTo(1.362)
expect(nftinfo[1].apr).toBeCloseTo(0.016)
expect(nftinfo[1].roundYield).toBeCloseTo(0.016)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should assert that is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's quite a bit we need to improve with the tests and we also need to set up a CI. I made an issue for that: #47

Copy link
Member

Choose a reason for hiding this comment

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

I have updated the tests to pass (they weren't)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

Copy link
Member

@idiom-bytes idiom-bytes left a comment

Choose a reason for hiding this comment

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

Generally looks good.

Lots of reformatting changes.
Let's make sure to setup a linter.

Copy link
Member

@KatunaNorbert KatunaNorbert left a comment

Choose a reason for hiding this comment

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

LGTM!

@jamiehewitt15 jamiehewitt15 merged commit 995fc9f into main Mar 28, 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.

Update APR => roundYield

3 participants