-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add mpl-core account indexing #178
Conversation
@@ -95,6 +96,11 @@ pub struct AssetMetadataAccountColumns { | |||
pub royalty_amount: i32, | |||
pub asset_data: Option<Vec<u8>>, | |||
pub slot_updated_metadata_account: u64, | |||
pub plugins: Option<Value>, |
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.
Might be cleaner to store in a different table?
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.
Yeah I could do that, its probably better then prefixing them with mpl_core_
in the main asset
table.
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.
Tough balance between normalization and query speed. Right now though the model is complex the read times are quick because its as simple index lookup. I'd like to keep that read speed.
The interface on the asset maps the plugins to a specific protocol if another pluggable asset type is creating in the future.
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.
I am also happy keeping the new columns in the asset table. I do think it its better for the query speed.
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.
I did draft it up using separate mpl_core
table here: #180
As you guys have pointed out it will add the extra queries so we will skip it.
cdd6a48
to
2269810
Compare
let update_authority = match asset.update_authority { | ||
UpdateAuthority::Address(address) => address.to_bytes().to_vec(), | ||
UpdateAuthority::Collection(address) => { | ||
find_collection_authority(conn, address.to_bytes().to_vec(), RETRY_INTERVALS) |
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.
I find the retry logic might not be a good idea here. We may slow down the indexing while retrying to index the account. Isn't this going to result in a race or may be a different state if collection authority isn't indexed yet?
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.
yes it could slow down indexing, but honestly not sure how to deal with it other than a retry. This approach is similar to what token metadata indexing uses to look for the token.
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.
In real life, for mpl-core the collection will have to exist before the assets are minted to the collection or if the asset is added to the collection, so this should not fail as long as concurrent process indexing got the collection.
.one(conn) | ||
.await?; | ||
|
||
for interval in retry_intervals { |
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.
Should this loop be at the beginning? Seems like there's a sleep without actually fetching the authority after waking up. This will result in hitting the max retry every time
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.
You are right I messed up this logic when copying it over from toke metadata and changing it. I think I was trying to make it more like a do-while but didn't complete it properly. Will fix now.
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.
Fixed in #181
Notes
Assets
andCollections
using account updates.MplCoreAsset
andMplCoreCollection
to deal with fact that they are slightly different types and must be selectable bysearch_assets
.getAsset
for an mpl-coreAsset
.getAsset
for an mpl-coreCollection
.getAssetsByAuthority
getAssetsByGroup
getAssetsByOwner
There's a couple scenarios which are hard to write integ tests for. These scenarios are described below and were tested locally with the Docker container:
"unknown_plugins"
section and is output as base64, which would allow users to parse it if they have an updated JS client. See below for an example.unknown_plugins
column is notNULL
.Related blockbuster PR
metaplex-foundation/blockbuster#41
Example output for an mpl-core
getAsset
with known and unknown plugins: