Skip to content

Conversation

@hans-kessock
Copy link
Contributor

Description

You may now view OCR2 keys on the same Key Management UI as the other keys, following the same structures and patterns as the original OCR keys (except with different data of course)

Please note that OCR2 key creation is not supported at this time as OCR2 keys require the specification of a chain family and that list is now, as far as any client is concerned, dynamically generated by the core node - this usually mean a simple query, but with the very soon to be added support for blockchain plugins, I am very leery of exposing this from the node just yet

There is also an addition to resolve the unhandled session timeout issue (exhibited by the Key Management page) that stems from that screen/view not having a query that would trigger session timeout at the appropriate level

Steps to Test

  1. yarn && yarn setup
  2. yarn start
  3. ...etc
  • [ x] This PR has an accompanying changeset if needed.

I don't like putting multiple things in a commit, but there's a redirect on authorization error addition that is part of the new OCR2KeysCard file - see the comments in the file
@hans-kessock
Copy link
Contributor Author

hans-kessock commented Feb 27, 2023

CI Tests are breaking now because of the resolver code being only aware of EVM and SOLANA (as well as being case sensitive which is problematic because they are stored by the keystore in lowercase I believe.)

I can update the resolver to be aware of starknet, but that is just kicking the can down the road because the core is moving away from hardcoded knowledge about chain families (which is why the OCR2 key management support I'm adding does not yet support creation of OCR2 keys.)

We might want to remove chain family name validation from the CI tests until this is resolved architecturally on the core node side; although, it will likely no longer be a part of the graphql generated definitions going forward - but that's not certain yet.)

@jkongie / @HenryNguyen5 - what do y'all think? I'm a newbie here at Chainlink, so feel free to club away ;)

@jkongie
Copy link
Contributor

jkongie commented Feb 28, 2023

I believe that you may to update the GQL schema with the new type anyway due to how the typecheck works, just to get this in. Since you are using starknet in the tests as a value, you will need to ensure that the frontend is compatible with the backend enum declaration.

@hans-kessock
Copy link
Contributor Author

hans-kessock commented Feb 28, 2023

@jkongie - that's the weirdness though. The backend graphql enum is all caps. The OCR2 key family is created/carried/persisted lowercase, and driven by a different enum.

I'll double check that the query doesn't perform any mapping/transformations when bundling the key data. If it does, I'll update the enumeration - otherwise it is indicative that the enumeration names and the keybundle values have been disconnected all along for OCR2 keys.

The OCR2 client side graphql bundling code already handles 'starknet' and other lowercase names just fine, it's only breaking in the CI tests.

I can modify my tests to avoid names it isn't aware of - since the real world usage doesn't use the forms the tests expect anyhow - but I may change the enumeration instead if it doesn't break other things.

Here's the mismatch in core (that will shortly get worse with a growth in chains):

This is used by the core for key creation:

const ( // EVM for Ethereum or other chains supporting the EVM EVM ChainType = "evm" // Solana for the Solana chain Solana ChainType = "solana" // StarkNet for the StarkNet chain StarkNet ChainType = "starknet" ) var SupportedChainTypes = ChainTypes{EVM, Solana, StarkNet}

This is what the graphql resolver defines (and is a bit out of date :) )

const ( // OCR2ChainTypeEVM defines OCR2 EVM Chain Type OCR2ChainTypeEVM = "EVM" // OCR2ChainTypeSolana defines OCR2 Solana Chain Type OCR2ChainTypeSolana = "SOLANA" )

@HenryNguyen5
Copy link
Contributor

Hey Hans,
You could create a useQueryErrorHandler hook that is a copy/paste of the useMutationErrorHandler, then, you can either:

Use the onError callback handler of useOCR2KeysQuery to sign out and redirect on UNAUTHORIZED apollo errors like so:

const handleQueryErrors = useQueryErrorHandler()
const { data, loading, refetch } = useOCR2KeysQuery({
    fetchPolicy: 'network-only',
    onError: handleQueryErrors
  })

or

Call the handler directly

const handleQueryErrors = useQueryErrorHandler()
const { data, loading, refetch, error } = useOCR2KeysQuery({
    fetchPolicy: 'network-only',
    onError: handleQueryErrors
  })
handleQueryErrors(error)

Note that we make our own handler since the error handling implementation of queries vs mutations may diverge over time.
You will notice that we will have 3 error handling hooks afterwards:

  • useErrorHandler
  • useMutationErrorHandler
  • useQueryErrorHandler

It may be wise to refactor the 3 hooks to extract common functionality, at a glance, they do the same logic when handling unauth errors.

If i understand you correctly, this will give you the intended result of notifying users on query error within the parent container component without having the child view component do that handling (edited)

@george-dorin
Copy link
Contributor

Hey Hans, You could create a useQueryErrorHandler hook that is a copy/paste of the useMutationErrorHandler, then, you can either:

Use the onError callback handler of useOCR2KeysQuery to sign out and redirect on UNAUTHORIZED apollo errors like so:

const handleQueryErrors = useQueryErrorHandler()
const { data, loading, refetch } = useOCR2KeysQuery({
    fetchPolicy: 'network-only',
    onError: handleQueryErrors
  })

or

Call the handler directly

const handleQueryErrors = useQueryErrorHandler()
const { data, loading, refetch, error } = useOCR2KeysQuery({
    fetchPolicy: 'network-only',
    onError: handleQueryErrors
  })
handleQueryErrors(error)

Note that we make our own handler since the error handling implementation of queries vs mutations may diverge over time. You will notice that we will have 3 error handling hooks afterwards:

  • useErrorHandler
  • useMutationErrorHandler
  • useQueryErrorHandler

It may be wise to refactor the 3 hooks to extract common functionality, at a glance, they do the same logic when handling unauth errors.

You could create a useQueryErrorHandler hook that is a copy/paste of the useMutationErrorHandler, then, you can either

Added in 0e400df (#26)

@george-dorin
Copy link
Contributor

As far as I know, starknet support for GQL is not going to be implemented any time soon so we should ignore it for now

@george-dorin george-dorin requested a review from jkongie March 16, 2023 12:58
@george-dorin george-dorin merged commit 67c1a28 into main Mar 17, 2023
@github-actions github-actions bot mentioned this pull request Sep 5, 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.

4 participants