Add NFT owner to API Server response #1858
Conversation
OBorce
commented
Dec 24, 2024
OBorce
commented
- Truck NFT owner changes in API Server
- Add NFT owner to nft endpoint response
OBorce
requested review from
erubboli,
azarovh and
ImplOfAnImpl
as code owners
OBorce
force-pushed
the
feature/api-server-nft-owner
branch
from
93d2e5e to
8619f18
Compare
OBorce
force-pushed
the
feature/api-server-nft-owner
branch
from
8619f18 to
d42c687
Compare
| if let Some(by_height) = self.nft_token_issuances.get_mut(&token_id) { | ||
| let last = by_height.values().last().expect("not empty"); | ||
| let owner = (amount > Amount::ZERO).then_some(address.as_object().clone()); | ||
| let new = NftWithOwner { | ||
| nft: last.nft.clone(), | ||
| owner, | ||
| }; | ||
| by_height.insert(block_height, new); | ||
| }; | ||
| } |
There was a problem hiding this comment.
- Let's add an assertion that the amount is either 0 or 1?
- You probably shouldn't update the map if the amount is zero. Currently this only works because
update_tables_from_transaction_inputsis called beforeupdate_tables_from_transaction_outputs(as I can see), IMO it's not good for this function to depend on this. - In
update_tables_from_transaction_outputs, in theTxOutput::IssueNftcase,set_nft_token_issuanceis called afterincrease_address_amount. How is it supposed to work?
There was a problem hiding this comment.
- we are updating in both input (decrease_amount, i.e. amount 0 case) and output (increase_amount) as a nft utxo can be used as an input but not transferred anywhere or burned, in that case we set the owner to None.
- as the NFT id will not be in the database the update owner will do nothing. If the order is for some reason reversed the on conflict will just update the owner to the same owner.
There was a problem hiding this comment.
2. we are updating in both input (decrease_amount, i.e. amount 0 case) and output (increase_amount) as a nft utxo can be used as an input but not transferred anywhere or burned, in that case we set the owner to None.
I see. A comment would be nice though.
But if calls to update_tables_from_transaction_outputs and update_tables_from_transaction_inputs are swapped, things would break, right? Asking this because I've just tried swapping them and all tests passed. Probably some tests are missing?
3. as the NFT id will not be in the database the update owner will do nothing. If the order is for some reason reversed the on conflict will just update the owner to the same owner.
Yeah I missed that, thanks.
There was a problem hiding this comment.
If we swap updating first the outputs then the inputs it will break only in a specific case where someone sends it from the same address to the same address. in that case the last decrease will set the owner to None. Will add a test for that just in case anyone swaps the processing order in the future.
| self.update_nft_owner(token_id, height, (amount > Amount::ZERO).then_some(address)) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn update_nft_owner( | ||
| &mut self, | ||
| token_id: TokenId, | ||
| height: i64, | ||
| owner: Option<&Address<Destination>>, | ||
| ) -> Result<(), ApiServerStorageError> { | ||
| self.tx | ||
| .execute( |
There was a problem hiding this comment.
Same comments as for the other update_nft_owner.
