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

P-261 Refactor Achainable type definitions #2464

Merged
merged 10 commits into from
Feb 15, 2024

Conversation

grumpygreenguy
Copy link
Contributor

@grumpygreenguy grumpygreenguy commented Feb 5, 2024

Context

  • Reduce some repetition in Achainable request type defs.
  • Use the BoundedWeb3Network type for the chain param everywhere

How (Optional)

Initial proposal was to extract the common fields to a struct, but they don't really seem to make semantic sense as a single type. Extracted them as a macro instead, which also allowed a bunch of repeated derive annotations to be deduplicated.

TODO

  • Validate approach
  • Fix resulting type errors from incompatible chain param type
  • Fix TS tests

@grumpygreenguy grumpygreenguy added the C0-breaking Breaking change that will cause existing client to break label Feb 5, 2024
Copy link

linear bot commented Feb 5, 2024

@grumpygreenguy grumpygreenguy marked this pull request as ready for review February 7, 2024 13:40
@grumpygreenguy grumpygreenguy requested review from Kailai-Wang, zhouhuitian and felixfaisal and removed request for Kailai-Wang February 7, 2024 13:49
@@ -255,7 +256,7 @@ impl TryFrom<AchainableParams> for Params {
let network = &p.chain;
let amount = ap.to_string(&p.amount)?;

let p = ParamsBasicTypeWithAmount::new(name, network, amount);
let p = ParamsBasicTypeWithAmount::new(name, &network[0], amount);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zhouhuitian @Kailai-Wang in the (existing) AmountToken case we're just using the first chain in the vector; is that a reasonable solution for the rest of the assertions, or should we make a more significant fix?

Copy link
Collaborator

@Kailai-Wang Kailai-Wang left a comment

Choose a reason for hiding this comment

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

Thanks! I invite @zhouhuitian to review it too

@Kailai-Wang
Copy link
Collaborator

Kailai-Wang commented Feb 9, 2024

@zhouhuitian will be back in one week (probably) - so we could merge it before it dangles for too long and make retroactive changes later if required.

However, we need to be very cautious about releasing and deploying it. We'll need confirmation from @zhouhuitian and ideally client colleagues (@jonalvarezz and @YoshiyukiSakura) to make sure we don't break anything for the current VC request construction.

Copy link
Contributor

@jonalvarezz jonalvarezz left a comment

Choose a reason for hiding this comment

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

Nice!

From the client side, I don't expect many changes as Assertion types are only used for requesting and since the enum isn't changing, it seems to be scoped down to the Web3Network -> Vec<Web3Network> change.

I created this ticket to follow up and adjust client-sdk correspondingly: https://linear.app/litentry/issue/P-514/client-sdk-update-types-accordingly-after-achainable-types-rework

@grumpygreenguy grumpygreenguy enabled auto-merge (squash) February 15, 2024 14:33
@grumpygreenguy grumpygreenguy merged commit eefbd20 into dev Feb 15, 2024
30 checks passed
@grumpygreenguy grumpygreenguy deleted the p-261-merge-achainable-request-structure branch February 16, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C0-breaking Breaking change that will cause existing client to break
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants