-
Notifications
You must be signed in to change notification settings - Fork 144
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
feat(rpc): implement Filecoin.EthGetStorageAt #4340
Conversation
src/rpc/methods/eth.rs
Outdated
params, | ||
..Default::default() | ||
}; | ||
let mut api_invoc_result = None; |
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.
nit: could be done withoutmut
.
let api_invoice_result = {
for ts in ... {
match {
Ok(res) -> return Some(res)
}
None
}
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.
The return
clause works for the function instead of the scope unless the scope is converted to a lambda. I was thinking about break Some(res);
syntax just like in some other languages but it does not work in 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.
If you want to avoid mut
here, you still could use labelled blocks, e.g.:
let api_invoc_result = 'block: {
for ts in ts.chain_arc(ctx.store()) {
match ctx.state_manager.call(&message, Some(ts)) {
Ok(res) => {
break 'block res;
}
Err(e) => tracing::warn!(%e),
}
}
return Err(anyhow::anyhow!("Call failed").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.
@elmattic Didn't know about this. This is great, thanks!
let mut with_padding = vec![0; 32_usize.saturating_sub(ret.len())]; | ||
with_padding.append(&mut ret); | ||
Ok(EthBytes(with_padding)) | ||
} 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.
nit: return early on if
, then there is no need for 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.
Personally, I feel using early returns here is not Rustic given rust is expression-based
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 guess it boils down to personal preference, however if .. else
nesting here does not read very well.
Imagine if every if in this method would have an else. Non-blocking.
src/rpc/methods/eth.rs
Outdated
Err(e) => tracing::warn!(%e), | ||
} | ||
} | ||
let Some(api_invoc_result) = api_invoc_result 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.
Looking at this more I'm thinking: why do we need Option
for api_invoc_result
? It would be much easier to use Result
. Then there is no need for let Some..
Something like:
for ... {
match {
Ok(res) -> return Ok(res)
}
}
return Err(err)
and then just do `api_invoc_result?`
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 don't think the return
clause works this way. #4340 (comment)
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.
Fixed.
src/rpc/methods/eth.rs
Outdated
let Some(msg_rct) = api_invoc_result.msg_rct else { | ||
return Err(anyhow::anyhow!("no message receipt").into()); | ||
}; | ||
if !api_invoc_result.error.is_empty() { |
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.
It it actually possible for !api_invoc_result.error.is_empty
when msg_rct
is Some
?
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.
Logically yes, in practice, I'm not sure. I think it does not hurt to check both and make the logic look more robust
src/rpc/methods/eth.rs
Outdated
let mut ret = fvm_ipld_encoding::from_slice::<RawBytes>(msg_rct.return_data().as_slice())? | ||
.bytes() | ||
.to_vec(); | ||
if ret.len() < 32 { |
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.
If you mean to pad on an EVM word boundary, you can use the EVM_WORD_LENGTH
constant.
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.
Fixed.
src/rpc/methods/eth.rs
Outdated
ctx: Ctx<impl Blockstore + Send + Sync + 'static>, | ||
(eth_address, position, block_number_or_hash): Self::Params, | ||
) -> Result<Self::Ok, ServerError> { | ||
let make_empty_result = || EthBytes(vec![0; 32]); |
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.
If you mean to return a zeroed EVM word, you could use the EVM_WORD_LENGTH
constant.
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.
Fixed.
for ts in ts.chain_arc(ctx.store()) { | ||
match ctx.state_manager.call(&message, Some(ts)) { | ||
Ok(res) => { | ||
break 'invoc res; |
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.
The loop label here is unnecessary. Non-blocking.
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.
Could you elaborate? I can compile the code without the loop label
error[E0571]: `break` with value from a `for` loop
--> src/rpc/methods/eth.rs:1298:25
|
1295 | for ts in ts.chain_arc(ctx.store()) {
| ----------------------------------- you can't `break` with a value in a `for` loop
...
1298 | break res;
| ^^^^^^^^^ can only break with a value inside `loop` or breakable block
|
help: use `break` on its own without a value inside this `for` loop
|
1298 | break;
| ~~~~~
Summary of changes
Changes introduced in this pull request:
implement Filecoin.EthGetStorageAt
Reference issue to close (if applicable)
Closes #4341
Other information and links
Change checklist