APY Finance adapter + Token Allocation #220

RodrigoAD · 2021-01-12T12:21:46Z

This PR includes two new adapters:

token-allocations adapter. This adapter could be used for price data relying adapters, such as Defi Pulse or Synthx
apy-finance adapter. Calculates the Total Value Locked in APY Finance.

To not make this PR really big, Defi Pulse migration to use token-allocations adapter will be added in a separate PR.

github-actions · 2021-01-12T12:24:57Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

token-allocation/README.md

boxhock · 2021-01-14T09:58:24Z

token-allocation/src/config.ts

+import cryptocompare from './data-providers/cryptocompare'
+import nomics from './data-providers/nomics'
+import coinpaprika from './data-providers/coinpaprika'
+import coinmarketcap from './data-providers/coinmarketcap'
+import coingecko from './data-providers/coingecko'
+import coinapi from './data-providers/coinapi'
+import amberdata from './data-providers/amberdata'
+import kaiko from './data-providers/kaiko'


Do we have no way of using the existing adapters instead of re-writing them here?

When we migrate the price adapters to .ts, token-allocation will be using them. Right now we can't

Everything under data-providers/ is exactly the same on defi-pulse, but this will use token-allocation adapter after this is merged, so all the repeated code will be removed from defi-pulse

Synthx could use token-allocation as well, so the repeated code will be removed there too.

Thanks for the explanation! Just worried about the technical debt.

Do we have a story for this migration? I feel like we should also include some work on the CVI adapter (which isn't even merged in yet, but I think it can use the same work once it's in).

I included 2 stories yes (#176484089 and #176464064). You can include the story for CVI too so we don't forget

boxhock · 2021-01-15T12:47:47Z

token-allocation/README.md

+## Input Params
+
+- `components`: Array of the token symbols. 
+- `units`: Array of balances in wei of each token.


Would it make sense to make this optional, and default it to an array of 1s?

boxhock · 2021-01-15T12:50:38Z

token-allocation/README.md

+      "10000000000000000000",
+      "10000000000000000000",
+      "10000000000000000000"


Does this match with the output in this readme?
And why does units change from wei as input to "Ether" in output?

It doesn't match you're right. Will fix it.

It would make sense to be the same units actually, either both 'wei' or both 'ether'. Wei I'd say makes more sense as it's more precise, but not sure.

I agree. On the last point, also consider that "ether" would make more sense normally, if you're not explicitly aware that this adapter takes "wei".

How much precision do we usually need with an adapter like this? I'm guessing there's a reason why you picked wei to begin with 😅

Haha there are some adapters that query the units from contracts directly, so they give it in wei. But they can be converted to "ether" of course 😄

Alright, sounds like the easiest thing to do is just give it in wei then.

boxhock · 2021-01-15T12:51:37Z

token-allocation/README.md

+
+- `components`: Array of the token symbols. 
+- `units`: Array of balances in wei of each token.
+- `currency` (optional). Currency we want the price on. `DEFAULT_CURRENCY` by default


More of a generic question, but would it be easy to extend this adapter with a param for "endpoint"?
Say the default currently is to get the prices, could it easily be extended to say marketcap percentage?

I don't think it would be hard, and make sense to have that functionality in this adapter.

…l-adapters-js into apy-tvl-adapter

boxhock

Would like some more 👀 on this, but looks good to me

krebernisak

@RodrigoAD here are some more suggestions you can work on in the next defi-pulse PR.

krebernisak · 2021-01-18T09:37:45Z

token-allocation/src/config.ts

+  return {
+    priceAdapter,
+    defaultCurrency,
+    makeDefaultUnits: makeDefaultUnits(defaultUnit || new Decimal(1e18).toString()),


Why is the default 1e18?

Even if we need an fn like this, and I think we can do without it, the config is probably not the place for it.

krebernisak · 2021-01-18T09:42:28Z

token-allocation/src/adapter.ts

+  units: Decimal
+  price?: number


Price can be Decimal to have better precision than number, but why is units typed as Decimal. IMO it should be number or BN?

krebernisak · 2021-01-18T09:50:15Z

token-allocation/src/adapter.ts

+  if (!index.every(isPriceSet)) throw new Error('Invalid index: price not set')
+  // calculate total value
+  return index // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+    .reduce((acc, i) => acc.plus(i.units.div(1e18).times(i.price!)), new Decimal(0))


We should not assume every token has 18 decimal points, for example wBTC does not.

We should take decimals on input, and use 18 as a default if not set.

krebernisak · 2021-01-18T10:01:04Z

composite/apy-finance/src/registry/index.ts

+type Allocations = {
+  components: string[]
+  units: string[]
+}


This type is a relic of defi-pulse underlying contract implementation and is inflexible. How about we change this type to something more usable like:

type TokenAllocation = { symbol: string decimals: number balance: string | BN }

This type should then be moved to the token-allocations adapter and used by APY & DeFi Pulse.

krebernisak · 2021-01-18T10:02:58Z

composite/apy-finance/src/registry/index.ts

+import registryAbi from './IRegistry.json'
+import assetAllocationAbi from './IAssetAllocation.json'
+
+const getRegistry = (address: string, rpcUrl: string, abi: any): ethers.Contract => {


This function is really unnecessary :)

krebernisak · 2021-01-18T10:08:14Z

composite/apy-finance/src/registry/IAssetAllocation.json

@@ -0,0 +1,53 @@
+[


Kind of doesn't make sense to have IAssetAllocation abi in a registry folder. Let's just put all ABIs in the /src/abi folder.

The second thing is these static ABIs are really hard to identify. What is the origin and source code for these ABIs? Where can we find some more documentation for these contracts? Please document answers to these questions.

krebernisak · 2021-01-18T10:19:44Z

token-allocation/src/adapter.ts

+  return components.map((component, i) => {
+    return {
+      asset: component,
+      units: new Decimal(new utils.BigNumber(units[i]).toString()),


Either Decimal or BigNumber, why both?

added token allocation and APY Fin adapter

d81a1e7

RodrigoAD requested review from krebernisak and justinkaseman January 12, 2021 12:21

RodrigoAD added 2 commits January 12, 2021 13:22

fix title on APY Finance readme

ce331a6

Merge remote-tracking branch 'origin/develop' into apy-tvl-adapter

ce64453

added changes to changelog

da90237

RodrigoAD changed the title ~~APY FInance adapter + Token Allocation~~ APY Finance adapter + Token Allocation Jan 12, 2021

RodrigoAD added 10 commits January 12, 2021 16:00

apy not aware of token allocations params

356432a

Merge branch 'develop' into apy-tvl-adapter

7f2ef22

using adapter factories

4b4a142

fixed typo strategy adapters

2763635

blockchain calls in parallel. Small refactor

5a7a337

fixed currency env var name

1cd9e85

added token allocation tests

d68ee23

Merge remote-tracking branch 'origin/develop' into apy-tvl-adapter

7b71378

fixed build

8d9ff7d

fix CMC new requirements

1fa4e25

RodrigoAD requested review from boxhock, ebarakos and emmick4 January 13, 2021 16:25

boxhock reviewed Jan 14, 2021

View reviewed changes

RodrigoAD added 3 commits January 14, 2021 11:26

cmc fixed aligned with similar adapters

b637eff

fixed no slug scenario in CMC

e6d94b9

Merge branch 'develop' into apy-tvl-adapter

6ec5518

boxhock self-requested a review January 15, 2021 12:44

boxhock reviewed Jan 15, 2021

View reviewed changes

RodrigoAD added 3 commits January 15, 2021 16:05

units are optional. result returned in wei

6c8e696

Merge branch 'apy-tvl-adapter' of github.com:smartcontractkit/externa…

c5e1799

…l-adapters-js into apy-tvl-adapter

fixed tests

128f24b

Merge remote-tracking branch 'origin/develop' into apy-tvl-adapter

3bf902d

boxhock approved these changes Jan 15, 2021

View reviewed changes

RodrigoAD merged commit 5823442 into develop Jan 15, 2021

krebernisak reviewed Jan 18, 2021

View reviewed changes

boxhock deleted the apy-tvl-adapter branch June 22, 2021 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APY Finance adapter + Token Allocation #220

APY Finance adapter + Token Allocation #220

RodrigoAD commented Jan 12, 2021

github-actions bot commented Jan 12, 2021

boxhock Jan 14, 2021

RodrigoAD Jan 14, 2021

boxhock Jan 14, 2021

RodrigoAD Jan 14, 2021

boxhock Jan 15, 2021

RodrigoAD Jan 15, 2021

boxhock Jan 15, 2021 •

edited

Loading

RodrigoAD Jan 15, 2021

boxhock Jan 15, 2021 •

edited

Loading

RodrigoAD Jan 15, 2021

boxhock Jan 15, 2021

boxhock Jan 15, 2021

RodrigoAD Jan 15, 2021

boxhock left a comment

krebernisak left a comment

krebernisak Jan 18, 2021

krebernisak Jan 18, 2021

krebernisak Jan 18, 2021

krebernisak Jan 18, 2021

RodrigoAD Jan 18, 2021

krebernisak Jan 18, 2021

RodrigoAD Jan 18, 2021

krebernisak Jan 18, 2021

krebernisak Jan 18, 2021

APY Finance adapter + Token Allocation #220

APY Finance adapter + Token Allocation #220

Conversation

RodrigoAD commented Jan 12, 2021

github-actions bot commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boxhock Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boxhock Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boxhock left a comment

Choose a reason for hiding this comment

krebernisak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boxhock Jan 15, 2021 •

edited

Loading

boxhock Jan 15, 2021 •

edited

Loading