add: ETCM-12351 dolos governed map data source #1109
Conversation
… into ETCM-12351-dolos-data-sources
AmbientTea
changed the title
| ), | ||
| ), | ||
| governed_map: Arc::new( | ||
| partner_chains_db_sync_data_sources::GovernedMapDataSourceCachedImpl::new( |
There was a problem hiding this comment.
I still see 2 datasources coming from dbsync here, StakeDistributionDataSourceImpl and CachedTokenBridgeDataSourceImpl
There was a problem hiding this comment.
Indeed, this is intentional.
At the beginning of this, we had dbsync based data sources (multiple).
To implement the dolos data sources, I copied the dbsync based data sources, and started replacing them one by one. This is necessary as we can't implement all 5-6 data sources all at once.
There is a transitional period, while the dolos data sources use a mixture of dolos and dbsync. We go through each source in the dolos data sources, and we rewrite the dbsync ones to use dolos.
Note, that all of this is in addition to the dbsync data source, which is kept as is.
Again, in addition to the dbsync data sources we created the dolos ones, which use pieces of the dbsync implementation as stand-in, until they are implemented one by one and replaced. This is needed because there is a lot of code and we don't want to do it all in one PR.
AmbientTea
left a comment
AmbientTea
left a comment
There was a problem hiding this comment.
Reading this PR, I can't help but think that all the code here is struggling very hard to do what a couple of SQL queries do relatively easily for Db-Sync data sources. It doesn't look like the API exposed by Dolos is well suited to our use case, as long as we can't narrow down our requests to a specific block range and are forced to run separate queries for each utxo.
Also a minor thing - the title mentions only governed map data sources, but implements 2 others as well. I would stick to 1 PR - 1 data source, unless they're trivial (they're not).
| self.paginated_request_all(&format!("addresses/{address}/utxos")).await | ||
| } | ||
|
|
||
| async fn addresses_utxos_asset( |
There was a problem hiding this comment.
This naming convention is kinda unwieldy IMO. I understand that it mirrors the request path, but it tells me nothing without looking into implementation. what about utxos_at_address_with_asset?
There was a problem hiding this comment.
It mirrors the function names in here. I'm not against renaming them.
There was a problem hiding this comment.
This is going to be outdated by the time it hits master if we go with the proposed change to use tx metadata. I would split it to another branch/PR and wait until we have a final decision on that.
| client.addresses_utxos_asset(icp_address.clone(), native_token.clone()).await?; | ||
|
|
||
| // Process each UTXO to calculate token deltas and gather transaction info | ||
| let futs = address_utxos.into_iter().map(|utxo| { |
There was a problem hiding this comment.
| let futs = address_utxos.into_iter().map(|utxo| { | |
| let futures = address_utxos.into_iter().map(|utxo| { |
| let address_utxos = | ||
| client.addresses_utxos_asset(icp_address.clone(), native_token.clone()).await?; |
There was a problem hiding this comment.
Am I understanding it right, that we're fetching all utxos at this address with this asses ever, and then filter out the ones before to_block and after checkpoint? If that's the best we can do with what API Dolos provides us with, I don't see a way this can scale on mainnet, especially for a bridge.
| let icp_address = icp_address.clone(); | ||
| async move { | ||
| let tx_hash = McTxHash::from_hex_unsafe(&utxo.tx_hash); | ||
| let tx = client.transaction_by_hash(tx_hash).await?; |
There was a problem hiding this comment.
Running a request for each utxo is not going to scale either.
| let native_token = native_token.clone(); | ||
| let icp_address = icp_address.clone(); | ||
| async move { | ||
| let tx_hash = McTxHash::from_hex_unsafe(&utxo.tx_hash); |
There was a problem hiding this comment.
This "unsafe" is in the name for a reason - it will panic if tx_hash is not valid. You should use decode_hex instead and handle the error. Returning Ok(None) with an optional warning log would be fine I think.
| let block_number = | ||
| McBlockNumber(block.height.unwrap_or_default().try_into().unwrap_or(0u32)); |
There was a problem hiding this comment.
block_by_id should return a structure with well-typed fields already. These types from the blockfrost client are terrible.
|
|
||
| for utxo in utxos { | ||
| // Check if this UTXO was created before or at target block | ||
| let tx_hash = McTxHash::from_hex_unsafe(&utxo.tx_hash); |
There was a problem hiding this comment.
unsafe again. We should get this data already validated and typed from our client
| if let Some(datum_hex) = &utxo.inline_datum { | ||
| match PlutusData::from_hex(datum_hex) { | ||
| Ok(plutus_data) => match GovernedMapDatum::try_from(plutus_data) { | ||
| Ok(GovernedMapDatum { key, value }) => { | ||
| mappings.insert(key, value); | ||
| }, | ||
| Err(err) => { | ||
| log::warn!("Failed to parse GovernedMapDatum: {}", err); | ||
| }, | ||
| }, | ||
| Err(err) => { | ||
| log::warn!("Failed to parse PlutusData from hex: {}", err); | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Possible helper function to parse GovernedMapDatum
| let (_, stake_address_hash_raw) = bech32::decode(&row.stake_address)?; | ||
| match &stake_address_hash_raw[..] { | ||
| [0xe0 | 0xe1, rest @ ..] => Ok(DelegatorKey::StakeKeyHash( | ||
| rest.try_into().expect("infallible: stake_address_hash_raw is 29 bytes"), |
There was a problem hiding this comment.
We should handle this by returning Err.
Resolved conflicts: - demo/node/src/data_sources.rs: Kept Dolos implementations for governed_map and bridge - toolkit/data-sources/dolos/src/lib.rs: Removed feature gate from block module - toolkit/data-sources/dolos/src/client/api.rs: Kept addresses_utxos_asset method
Seems like #1081 got in here as well. Master got merged into this branch, but the base branch is not master on the PR. 🤷 |
…o ETCM-12351-dolos-governed-map-data-source
kpinter-iohk
changed the base branch from
dolos-data-source-bridge-jankun
to
master
Description
Implemented governed map dolos data source and tweaked the stake distribution code. Confirmed that governed map, bridge and stake distribution sources from dolos is working on the local environment.
Checklist
changelog.mdfor affected crate