-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: P-212 Club3 NFT VC #2492
feat: P-212 Club3 NFT VC #2492
Conversation
added new evm network support for polygon and arbitrum, pls see how client side wanted to extend the supported network. @YoshiyukiSakura |
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.
Hey @higherordertech
Not related to the PR directly, But i see these crates assertion-build-v2
and credential-v2
,
- I may have missed out why we are creating separate crates for different versions?
- Do we plan on continuing to iter this way, wherein we have
v3
and thenv4
? - If not are we planning on merging
assertion-build-v2
andassertion-build
, So what we waiting for in order to merge?
We have existing feature gating and version controlling features in Rust without having to explicitly define v2
crates and so on.
The plan is to migrate all v1 credentials under v2 and then v1 can be deprecated and v2 can be renamed to be the only version. Currently they co-exists because the migration work is not prioritised yet (while we are still catching new VC requirement), while at same time some of the new VC can not really be implemented via the v1 structure. |
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.
Not much to complain from my side :)
I've been wondering the difference between lc-credential and lc-crediential-v2
tee-worker/local-setup/.env.dev
Outdated
@@ -31,6 +31,8 @@ ACHAINABLE_AUTH_KEY= | |||
ONEBLOCK_NOTION_KEY= | |||
NODEREAL_API_KEY= | |||
GENIIDATA_API_KEY= | |||
GENIIDATA_API_KEY=142cf1b0-1ca7-11ee-bb5e-9d74c2e854ac |
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.
We have this key already - see the line above
let api_key = data_provider_config.moralis_api_key.clone(); | ||
let api_retry_delay = data_provider_config.moralis_api_retry_delay; | ||
let api_retry_times = data_provider_config.moralis_api_retry_times; | ||
let api_url = data_provider_config.moralis_api_url.clone(); |
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 out of this PR's scope, but sooner or later we'll need a refactoring to have a trait-based impl. The background is I see a lot of duplicate code, we could have something like build_client_with auth
or build_client_with retry
- or as fn parameters. 🤔
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.
added fail fast boolean option in service layer, and trait in data provider layer now
e0e3b45
to
aa43143
Compare
With minimum changes, also added RetryableError for client to differentiate retryable error or non retryable error:
|
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.
LGTM
e48d982
to
4b9fd13
Compare
Asking for reviews from @BillyWooo @zhouhuitian |
4b9fd13
to
b19386d
Compare
…on network, added general nft holder VC using this new data provider, added Club3 VC, migrated existing WeirdoGhostGang VC under it
b19386d
to
993a002
Compare
@Kailai-Wang let me take this chance to explain the differences: lc-credentials-v2 does not depend on lc-data-providers, and doesn't depend on any data provider from the credential level, nor limit how many data provider can be used. e.g. there won't be so called achainable credential or noderal credentail, instead a credential is just a credential decoupled from any (one or many) data providers it would require to use. while lc-credentials depend on lc-data-providers and has always hard coded the data provider dependency in credential level, and also constraint one credential can only use max one data provider |
Context
1 added new data provider morails (other existing data provider does not support the required evm network)
2 added arbitrum and polygon network
3 added general nft holder VC using this new data provider
4 added Club3 VC
5 migrated existing WeirdoGhostGang VC under it
Labels
Please apply following PR-related labels when appropriate:
C0-breaking
: if your change could break the existing client, e.g. API change, critical logic changeC1-noteworthy
: if your change is non-breaking, but is still worth noticing for the client, e.g. reference code improvementHow (Optional)
Testing Evidences
Please attach any relevant evidences if applicable