Ajw/164 tx blockfrost api #370
Conversation
- returns a tiny bit of transaction info given a tx hash
alexwoods
requested review from
SupernaviX,
sandtreader and
whankinsiv
and removed request for
whankinsiv
whankinsiv
left a comment
whankinsiv
left a comment
There was a problem hiding this comment.
Looks great! Just a few fields whose names don't align with the BF schema.
common/src/queries/transactions.rs
Outdated
| #[derive(Debug, Clone, serde::Deserialize)] | ||
| pub enum TransactionOutputAmount { | ||
| Lovelace(Lovelace), | ||
| Asset(NativeAsset), | ||
| } | ||
|
|
||
| impl Serialize for TransactionOutputAmount { |
There was a problem hiding this comment.
If a type implements both Serialize and Deserialize, they should match; otherwise the data can't round-trip. Would be better to either
- have a different struct specifically in
rest_blockfrostwhich deserializes correctly - make
TransactionOutputAmountinto a struct withunitandamountfields, so (de)serialization works right - not implement
Deserializehere (and we make caryatid not depend on it [obvs lots of extra work there])
There was a problem hiding this comment.
I'm going for a 4th option, which is to apply the standard Serialize derive in common, and move the special serialization into rest_blockfrost. This is one of the cases where the serialization for blockfrost is kind of unusual due to the numerics being serialized to strings. It makes some sense for JSON, but for CBOR it's not good. I think we would benefit from picking a default serialization target for common structs, and I think it should probably be CBOR. Shout if you think this is the wrong approach
There was a problem hiding this comment.
I should probably refactor these to return Result<N> at some point 😓
@whankinsiv Do you have a tool for checking these? |
sandtreader
left a comment
sandtreader
left a comment
There was a problem hiding this comment.
Massive piece of work, good job!
Only queries are about possible missing Conway certs where you can register and delegate at the same time, and whether counting PoolRegistrations is enough for deposits.
| stake_cert_count += 1; | ||
| } | ||
| conway::Certificate::StakeDelegation { .. } => delegation_count += 1, | ||
| _ => (), |
There was a problem hiding this comment.
I think there are a bunch of other Conway cert types that need to be counted here - Reg/UnReg/StakeRegDeleg etc.
| registration: false, | ||
| }); | ||
| } | ||
| _ => (), |
There was a problem hiding this comment.
Also here, there are combined register and delegate certs like StakeRegDeleg, VoteRegDeleg, StakeVoteRegDeleg (see handling in map_parameters.rs)
| }); | ||
| } | ||
| } | ||
| _ => (), |
| network_id.clone(), | ||
| false, | ||
| )?, | ||
| active_epoch: tx.block.extra.epoch + 1, |
There was a problem hiding this comment.
If valid (I assume it is!) needs a comment as to why incremented
|
|
||
| match param { | ||
| None => handle_transaction_query(context, tx_hash, handlers_config).await, | ||
| Some("utxo") => Ok(RESTResponse::with_text(501, "Not implemented")), |
There was a problem hiding this comment.
Is there a ticket to do this?
| } | ||
| } | ||
|
|
||
| /// Handle `/txs/{hash}` |
There was a problem hiding this comment.
Actually /txs/{hash}[/param][/param2]] I think? (to differentiate from the next fn)
| } | ||
| Some("metadata") => match param2 { | ||
| None => handle_transaction_metadata_query(context, tx_hash, handlers_config).await, | ||
| Some("cbor") => Ok(RESTResponse::with_text(501, "Not implemented")), |
| // TODO: calc from outputs and inputs if recorded_fee is None | ||
| let fee = txs_info.recorded_fee.unwrap_or_default(); | ||
| let deposit = match calculate_deposit( | ||
| txs_info.pool_update_count, |
There was a problem hiding this comment.
Worried about this - check if deposit is paid for subsequent updates of the same pool reg.
There was a problem hiding this comment.
(I have no idea what to do if it isn't, since it requires SPO state)
| Err(e) => return Some(Err(e)), | ||
| }; | ||
| // TODO: calc from outputs and inputs if recorded_fee is None | ||
| let fee = txs_info.recorded_fee.unwrap_or_default(); |
There was a problem hiding this comment.
There is code to do this in common/transactions I think?
Description
This PR adds some of the txs endpoints to the REST Blockfrost module.
Related Issue(s)
Relates to #137, #164.
How was this tested?
Manual comparison against dbsync (mainnet) :-(
Example requests that should work on mainnet:
Checklist
Impact / Side effects
There shouldn't be any side-effects.
Reviewer notes / Areas to focus
I've made custom serialisers within the blockfrost code. I'm unsure if there's a nicer way to do what I was trying to do, which was to keep serialisation out of the common library because in many cases this serialisation is quite peculiar to blockfrost and should not be a default.