-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix: Gracefully handle CardanoTokenRegistry
errors during getAsset
request
#412
Conversation
818365f
to
d08353b
Compare
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.
Looks good so far @IvayloAndonov, just a minor change request
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.
Given there's currently only a single source for this metadata, let's revert the breaking AssetInfo
type change given tokenMetadata
=== undefined
can be used to infer the success of the registry request.
a3c34af
to
039e0f0
Compare
Rebased with master and resolved conflicts. Revert the breaking |
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.
DbSyncAssetProvider
test is missing the assertion that demonstrates the assetInfo.tokenMetadata
value is undefined
if the asset is not found in the token registry.
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.
Nice work! 🚀
938bf9c
09b85b3
to
938bf9c
Compare
Rebased with master and squashed commits. |
- lift up error handing into `DbSyncAssetProvider - return `undefined` if there is an error while fetch from token metadata server
938bf9c
to
32a9b1f
Compare
CardanoTokenRegistry
errors during getAsset
requestCardanoTokenRegistry
errors during getAsset
request
Context
AssetInfo.tokenMetatdata
is stored in an off-chain registry, which cannot be depended on for returning a match for the given assetIds, nor it's service availability. Currently we’re rethrowing request errors as a Provider error, which is not ideal since it then couples our own service health, as opposed to the desired behaviour of omitting the metadata from the response.cardano-js-sdk/CardanoTokenRegistry.ts at 8fe1cdd7683a7802bebd081616abead292488953 · input-output-hk/cardano-js-sdk
This issue does raise an important question of how can a client differentiate between a response that included a
404
from the registry vs a5xx
with our current shape of from AssetInfo.tokenMetadataProposed Solution
Approach A)
Introduce the suggested approach from the task description with minor diffs as follows:
Notes:
cached: boolean
is committed because it's not so related to theSource
scope, however, it could be added easily with correct values withinCardanoTokenRegistry
easilysource?
property is nullable to keep the compatibility with the other providers which implement sameAssetProvider
such asblockfrost
. Not sure whatsource.type
inblockfrost
should be since the tokenMetadata is fetched as part of the wholeblockfrost.assetsById()
response.cardano-services
is usedAssetInfo<TokenMetadataSource>
, everywhere else isAssetInfo<unknown>
data
obj and addedsource
obj incardano-services
)Approach B) Omitting the metadata from the response in case of 5xx error thrown by the external source (in our current situation - token metadata server)
Following the CIP-0035 comment, the second approach could be simply to avoid the
AssetInfo
interface changes and to keep it as it is:tokenMetadata?: TokenMetadata | null;
with the only difference is to shallow theProviderError
in catch block and to omittokenMetadata
in case of network error by the external token server by returning simplynull
.The downside here is losing the details of the source/origin of that data.
Important Changes Introduced