Add type info to oracle data #1184
Conversation
There was a problem hiding this comment.
⭐ the code all looks perfect but i think we need clarity on how this is intended to be used. Eg: if the type is a custom struct with several fields, how should this be expressed in the type definition?
This feels a bit the same as the schema definitions for programs - we have a field but no clear rules about how it should be filled in.
crates/test-cli/src/lib.rs
Outdated
| let headings = get_oracle_headings(&api, &rpc).await?; | ||
| Ok(serde_json::to_string_pretty(&headings)?) | ||
| let oracles_data = get_oracle_headings(&api, &rpc).await?; | ||
| Ok(format!("{:?}", oracles_data)) |
There was a problem hiding this comment.
Ideally, if cli.json is true, all output should be JSON. In this case it probably doesnt matter because i doubt the devops team need to use this in a script. But ideally we should do something like:
if cli.json {
Ok(serde_json::to_string_pretty(&oracles_data)?)
} else {
Ok(format!("{:?}", oracles_data))
}| OracleInfo { | ||
| oracle_data: BoundedVec::try_from(block_number.encode()) | ||
| .expect("Block number fits in bounded vec; qed"), | ||
| oracle_type: "u32".as_bytes().to_vec(), |
There was a problem hiding this comment.
Do we not need to say something like scale encoded u32? Or are we assuming that oracle_data types will always be scale encoded and implement Encode / Decode?
It seems strange that elsewhere we use JSON for program aux data / config, but use scale here. Using both would mean programs need to have dependencies for both json and scale.
If i just read u32 i would expect to use either u32::to_be_bytes() or u32::to_le_bytes().
There was a problem hiding this comment.
ye, I chose scale because it plays really nicely with substrate, I mean Im open for other optinons here, but pretty much we should force all oracle info data types to be the same type either scale or json (im still in favour of scale in substrate)
I see it as different as with programs anyone can build it and write it for themselves so they can use it personally and not need to get the schemas from anyone else. And if anyone else wants to use it, well they should already be going to a third party anyways to verify the code etc. With oracle data it is set by governance and set once for the whole chain usage. Thus governance can determine if the type is proper and there is no need to go to a third party anyways, so it is a closed loop. As for custom struct again this can only be set by governance so im all for cross that bridge when we get there |
There was a problem hiding this comment.
I disagree with putting schemas on-chain. It's pretty fragile, error prone, and expensive storage wise. For example, if we decide to change the BlockNumber type for whatever reason this code could break (this is just an example for the current code, but I assume you'll eventually want to add other types for which change might be more realistic).
I haven't followed the Oracle story very closely either, so I'm not entirely sure what the end goal with this is either. So maybe something you and I will have to catch up on.
| - In [#1184](https://github.com/entropyxyz/entropy-core/pull/1184/) The ```OracleData``` mapping now holds a | ||
| struct ```OracleInfo``` which includes the ```oracle_data``` and the ```oracle_type``` |
There was a problem hiding this comment.
For non-code blocks there should only be a single tick used.
| - In [#1184](https://github.com/entropyxyz/entropy-core/pull/1184/) The ```OracleData``` mapping now holds a | |
| struct ```OracleInfo``` which includes the ```oracle_data``` and the ```oracle_type``` | |
| - In [#1184](https://github.com/entropyxyz/entropy-core/pull/1184/) The `OracleData` mapping now | |
| holds a struct `OracleInfo` which includes the `oracle_data` and the `oracle_type` |
Adds a type to be added with oracle data to allow developers to know the expected type of the oracle data