Skip to content
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

Pr 824 integrate hydrating cache oracle chain #854

Conversation

netboz
Copy link
Contributor

@netboz netboz commented Jan 20, 2023

Description

This will address ticket #824 and
Fixes #824
Fixes #836

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

There is still some refinement needed.

How Has This Been Tested?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@netboz netboz added feature New feature request oracle chain Involve OracleChain core team Assigned to the core team labels Jan 20, 2023
@netboz netboz self-assigned this Jan 20, 2023
@netboz netboz requested review from Neylix, samuelmanzanera and bchamagne and removed request for Neylix January 20, 2023 09:14
config/dev.exs Outdated Show resolved Hide resolved
@netboz netboz force-pushed the pr_824_integrate_hydrating_cache_oracle_chain branch 2 times, most recently from d55e462 to bf3c8eb Compare January 23, 2023 10:31
@netboz netboz force-pushed the pr_824_integrate_hydrating_cache_oracle_chain branch 2 times, most recently from 024fd49 to 03d1749 Compare January 23, 2023 17:58
@netboz
Copy link
Contributor Author

netboz commented Jan 25, 2023

I need to do a final refinement, like to remove unused mocks and I would like to add metrics also, if possible.

@netboz netboz force-pushed the pr_824_integrate_hydrating_cache_oracle_chain branch from 24738bb to 8d71327 Compare February 6, 2023 19:43
@apoorv-2204 apoorv-2204 self-requested a review February 14, 2023 08:48
Copy link
Member

@bchamagne bchamagne left a comment

Choose a reason for hiding this comment

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

2 suggestions for me:

folder

there is an "archethic_cache" folder at root, I would move the HydratingCache in here.

config

I think there are 2 configurations mixed in 1.
Here you have a UCOPrice config that contains both providers & cache configuration (refreshrate/ttl).
Then in the Supervisor, you transform this into {MFA}.
I suggest to have the {MFA} directly in a HydratingCache config (adding services should not require code changes in the cache).

uco_providers =  [
  Archethic.OracleChain.Services.UCOPrice.Providers.CoinX,
  Archethic.OracleChain.Services.UCOPrice.Providers.CoinY,
  Archethic.OracleChain.Services.UCOPrice.Providers.CoinZ
  ]

config :archethic, Archethic.OracleChain.Services.UCOPrice,
  providers: uco_providers
  
config :archethic, Archethic.Utils.HydratingCache,
   [<some_other_cache>] ++ 
    Enum.map(uco_providers,&({&1, {&1, :fetch, [["eur", "usd"]]}, 60_000, :infinity}))

@apoorv-2204
Copy link
Contributor

2 suggestions for me:

folder

there is an "archethic_cache" folder at root, I would move the HydratingCache in here.

config

I think there are 2 configurations mixed in 1. Here you have a UCOPrice config that contains both providers & cache configuration (refreshrate/ttl). Then in the Supervisor, you transform this into {MFA}. I suggest to have the {MFA} directly in a HydratingCache config (adding services should not require code changes).

uco_providers =  [
  Archethic.OracleChain.Services.UCOPrice.Providers.CoinX,
  Archethic.OracleChain.Services.UCOPrice.Providers.CoinY,
  Archethic.OracleChain.Services.UCOPrice.Providers.CoinZ
  ]

config :archethic, Archethic.OracleChain.Services.UCOPrice,
  providers: uco_providers
  
config :archethic, Archethic.Utils.HydratingCache,
   [<some_other_cache>] ++ 
    Enum.map(uco_providers,&({&1, {&1, :fetch, [["eur", "usd"]]}, 60_000, :infinity}))

maybe we can create a behavior for cache. like @samuelmanzanera has done in Node KeyStore, shared secrets keystore for the
use of ets tables.

@bchamagne
Copy link
Member

I updated a few things as we discussed, I suggest to read commit by commit.

config/config.exs Outdated Show resolved Hide resolved
lib/archethic_cache/hydrating_cache.ex Outdated Show resolved Hide resolved
lib/archethic/oracle_chain/supervisor.ex Outdated Show resolved Hide resolved
Copy link
Member

@bchamagne bchamagne left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a slight comment about Archethic.OracleChain.ServiceCacheSupervisor which I believe is not helpful, but no harm here.

@samuelmanzanera
Copy link
Member

samuelmanzanera commented Mar 16, 2023

Looks good to me, just a slight comment about Archethic.OracleChain.ServiceCacheSupervisor which I believe is not helpful, but no harm here.

Yes, the point is to enforce implementation to leverage hydrating cache for the fetching.

@Neylix Neylix force-pushed the pr_824_integrate_hydrating_cache_oracle_chain branch from 9017cec to ef646d7 Compare March 24, 2023 09:23
@Neylix Neylix merged commit 0a37ad0 into archethic-foundation:develop Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core team Assigned to the core team feature New feature request oracle chain Involve OracleChain
Projects
None yet
6 participants