-
Notifications
You must be signed in to change notification settings - Fork 40
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
Index Token Extensions and Fungible Tokens #214
base: main
Are you sure you want to change the base?
Index Token Extensions and Fungible Tokens #214
Conversation
…egration test for the token extensions.
d0cfd16
to
d1855e0
Compare
1dea6a5
to
abe1adc
Compare
abe1adc
to
c22c4c2
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.
I'd like to see some more spec cases.
A few different token extensions and fungible examples.
...n_tests/snapshots/integration_tests__token_extensions_tests__token_extensions_get_asset.snap
Outdated
Show resolved
Hide resolved
will add more scenarios |
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.
grouping was recently activated on mainnet. After this was initially drafted can you look at how DAS is handling TE groupings? I didn't see any groupings in the scenerio.
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.
Great change! Approved with a few minimal comments
"withdraw_withheld_authority_elgamal_pubkey": [ | ||
28, | ||
55, | ||
230, | ||
67, | ||
59, | ||
115, | ||
4, | ||
221, | ||
130, | ||
115, | ||
122, | ||
228, | ||
13, | ||
155, | ||
139, | ||
243, | ||
196, | ||
159, | ||
91, | ||
14, | ||
108, | ||
73, | ||
168, | ||
213, | ||
51, | ||
40, | ||
179, | ||
229, | ||
6, | ||
144, | ||
28, | ||
87 |
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.
Commenting in test just to show the issue: I think we should print these Pubkey's in bs58 if possible. That would probably be a blockbuster or even more upstream change, and probably out of scope for this PR, but just mentioning here.
pub fn get_content(asset: &asset::Model, data: &asset_data::Model) -> Result<Content, DbErr> { | ||
match asset.specification_version { | ||
Some(SpecificationVersions::V1) | Some(SpecificationVersions::V0) => { | ||
v1_content_from_json(data) | ||
} | ||
Some(_) => Err(DbErr::Custom("Version Not Implemented".to_string())), | ||
None => Err(DbErr::Custom("Specification version not found".to_string())), | ||
} | ||
pub fn get_content(data: &asset_data::Model) -> Option<Content> { | ||
v1_content_from_json(data).ok() |
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.
Strange that there's a V2 that was never used. Maybe we should delete that but doesn't have to be part of this PR.
let edition_nonce = | ||
safe_select(chain_data_selector, "$.edition_nonce").and_then(|v| v.as_u64()); | ||
|
||
let (content, edition_nonce, basis_points, mutable, uses) = if let Some(data) = &data { |
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 like this cleanup putting it all together
digital_asset_types/src/rpc/asset.rs
Outdated
@@ -87,6 +96,10 @@ impl From<Interface> for (SpecificationVersions, SpecificationAssetClass) { | |||
SpecificationVersions::V1, | |||
SpecificationAssetClass::MplCoreCollection, | |||
), | |||
Interface::FungibleToken => ( | |||
SpecificationVersions::V0, |
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.
Probably doesn't matter based on its usage, but seems like it would be V1 as most other things are now V1.
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.
will change to V1
slot_updated_mint_account: Set(Some(columns.slot_updated_mint_account as i64)), | ||
supply_mint: Set(Some(columns.mint.clone())), | ||
slot_updated_mint_account: Set(Some(columns.slot_updated_mint_account)), | ||
slot_updated: Set(Some(columns.slot_updated_mint_account)), |
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.
Good fix thank you for fixing the plain slot_updated
field as well
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!
Add support to index fungibles along with non-fungibles , so ppl can query fungible tokens as well
Add support to return token extensions data ( along with mint and token_account data)