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

Projected NFT indexing #158

Merged
merged 14 commits into from
Dec 1, 2023
Merged

Projected NFT indexing #158

merged 14 commits into from
Dec 1, 2023

Conversation

gostkin
Copy link
Contributor

@gostkin gostkin commented Oct 31, 2023

No description provided.

@gostkin gostkin force-pushed the egostkin/index-projected-nft branch 3 times, most recently from 3dbe3f6 to e0d8cac Compare October 31, 2023 17:31
NotInlineDatum,
}

impl From<ProjectedNftOperation> for i32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if it's better or not, but you can use #[repr(i32)] on an enum assign the numbers in the definition, and then this would be just as i32

@gostkin gostkin force-pushed the egostkin/index-projected-nft branch from 9a24188 to 32de9cd Compare December 1, 2023 13:17
pub struct Model {
#[sea_orm(primary_key, column_type = "BigInteger")]
pub id: i64,
pub owner_address: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a Vec<u8> instead of a foreign key into the stake credential table (careful: there is no guarantee this staking key exists in the table)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be a Vec<u8> instead of a foreign key into the stake credential table (careful: there is no guarantee this staking key exists in the table)

as soon as there's no guarantee and this address is what we actually get from the plutus datum i think it makes sense to leave it like this

Comment on lines +10 to +12
pub previous_utxo_tx_hash: Vec<u8>,
#[sea_orm(column_type = "BigInteger", nullable)]
pub previous_utxo_tx_output_index: Option<i64>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these not be a foreign key to the output table?

pub hololocker_utxo_id: Option<i64>,
#[sea_orm(column_type = "BigInteger")]
pub tx_id: i64,
pub asset: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, we can use a foreign key for this asset as well

pub tx_id: i64,
pub asset: String,
#[sea_orm(column_type = "BigInteger")]
pub amount: i64,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's too ugly, but you can probably get the amount information implicitly from the fact you're already referencing the UTXO table

Comment on lines 335 to 342
return ProjectedNftData {
previous_utxo_tx_hash: vec![],
previous_utxo_tx_output_index: None,
address: vec![],
plutus_data: datum,
operation: ProjectedNftOperation::ParseError,
for_how_long: None,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really okay to return this in the error state? When does the error state actually happen (when people create a Projected NFT datum that actually contains garbage and wasn't generated with our library?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Comment on lines 306 to 313
return ProjectedNftData {
previous_utxo_tx_hash: vec![],
previous_utxo_tx_output_index: None,
address: vec![],
plutus_data: hash.to_vec(),
operation: ProjectedNftOperation::NotInlineDatum,
for_how_long: None,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the end result of returning this default object if they didn't provide the actual inline datum? We disable this in the smart contract, so we should at least have a comment saying this case is possible to see, but that these aren't spendable (not sure if this actually affects anything from the Carp side though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the object is not an inline datum the contract doesn't accept it and funds are locked forever

@@ -13,6 +13,7 @@ export interface ISqlBlockLatestResult {
hash: Buffer;
height: number;
id: number;
payload: Buffer | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this query wasn't regenerated before

};

export type ProjectedNftRangeResponse = {
actionSlot: number,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "actionSlot"

Keep in mind that when defining the data type, OpenAPI actually looks at the comments you put on these types (which is why we use alias like Address instead of string in the other models)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

action slot -- slot of a block at which the transaction happened

let datum = match datum_option {
DatumOption::Hash(hash) => {
return ProjectedNftData {
previous_utxo_tx_hash: vec![],
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably use ..Default::default() to shrink the code for a lot of these

@SebastienGllmt SebastienGllmt merged commit 30db1e7 into main Dec 1, 2023
3 checks passed
@SebastienGllmt SebastienGllmt deleted the egostkin/index-projected-nft branch December 1, 2023 15:22
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.

None yet

3 participants