-
Notifications
You must be signed in to change notification settings - Fork 987
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
Update block handler input data #1761
Conversation
70da35e
to
26b3851
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some critical issues here, and a few style nits.
.await | ||
{ | ||
Ok(block) => Ok(EthereumBlockType::FullWithReceipts(block)), | ||
Err(e) => Err(anyhow::anyhow!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using with_context
to add additional information to an error, rather than formatting the error. https://docs.rs/anyhow/1.0.3/anyhow/trait.Context.html#tymethod.with_context
7604df9
to
43a273a
Compare
Light, | ||
Full, | ||
FullWithReceipts, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use some comments on the meaning of each type. Also the naming of BlockType
vs EthereumBlockType
isn't great, maybe EthereumBlockType
would be a bit better as EthereumBlockData
, or something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that's a good point; I'd love to improve the naming, but I'm having trouble thinking of something better right now.
The obvious names are already in use: EthereumBlockData
, EthereumBlock
...
What about BlockData
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets see what this fields ends up being called in the manifest, and try to use a similar name.
pub hash: H256, | ||
pub parent_hash: H256, | ||
pub uncles_hash: H256, | ||
pub author: H160, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if Geth provides the author
field, it's worth double checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the gist comparing Geth vs. Parity structs the author
field is indeed omitted from Geth, but author
is included in the existing EthereumBlockData
struct that has been in use for a long time.
} | ||
|
||
impl From<EthereumBlockHandlerData> for String { | ||
fn from(data: EthereumBlockHandlerData) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a Display
impl, which allows .to_string()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used for serialization. I think the meaning of Display
is too loose a contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, for that purpose it would be nicer to implement this conversion for store::Value
.
@@ -344,7 +344,7 @@ impl ToAscObj<AscFullEthereumBlock> for FullEthereumBlockData { | |||
size: self | |||
.size | |||
.map(|size| heap.asc_new(&BigInt::from_unsigned_u256(&size))) | |||
.unwrap_or_else(|| AscPtr::null()), | |||
.into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping we could go even further here to change...
size: self
.size
.map(|size| heap.asc_new(&BigInt::from_unsigned_u256(&size)))
.into(),
to just...
size: self.size.into(),
Would whatever trait required to do that run afoul of the orphan rules? (If so, a new trait could be added).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... that would clean up this module, but it obfuscates the conversion to the asc data structure. Using into()
makes code navigation more difficult (breaks it in IntelliJ Rust.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this for your to decide, but I would have thought that if for example if u256
had an impl to AscPtr
that did heap.asc_new(&BigInt::From_unsigned_u256(value))
then it would be easy to build on top of that to just make anything that was Option<impl Into<AscPtr>>
call the base into and make it possibly nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLI is failing with this output:
error[E0412]: cannot find type `WasmInstanceContext` in this scope
6316 --> src/module/mod.rs:231:15
6317 |
63184 | assert_sync::<WasmInstanceContext>();
6319 | ^^^^^^^^^^^^^^^^^^^ not found in this scope
6320
6321error: aborting due to previous error
6322
6323For more information about this error, try `rustc --explain E0412`.
173ee3e
to
98dae83
Compare
- Rather converting to EthereumBlockType before sending the mapping request, pass EthereumBlockType down from process_block(). - Add utility functions to EthereumBlockType.
EthereumBlockHandlerData has the same variants, no need for both.
98dae83
to
b053022
Compare
@fordN is this PR still valid? I think there have been quite a few changes since |
I'll close this for now, @fordN feel free to re-open this 🙂 |
This PR contributes to a new feature that allows subgraph developers to choose between different Ethereum block structs to use as the input argument to a block handler mapping function.
Included:
dataSource > mapping > blockHandlers > input
where the input block type to the block handler function is defined, defaults toBlock
.FullBlock
andFullBlockWithReceipts
with parsing from string, corresponding Assemblyscript structs and conversions.FullBlockWithReceipts
input.Related PRs: