Skip to content
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

Xcalloptions refactor #19

Open
wants to merge 11 commits into
base: gwyneth-debug-shit
Choose a base branch
from

Conversation

CeciliaZ030
Copy link

No description provided.

Copy link

@Brechtpd Brechtpd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think current method is problematic because xcalloptions can be faked.

Maybe a new field for the xcalloptions inside PrecompileOutput could work, but also kind of hacky, so not necessarily better than current workaround.

Comment on lines 18 to 20
let version = u16::from_be_bytes(input[0..2].try_into().unwrap());
let chain_id = u64::from_be_bytes(input[2..10].try_into().unwrap());
println!("chain_id: {:?}", chain_id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah confusing diff because it seems you didn't pull my commit

let mut prefix = b"XCallOptions".to_vec();
prefix.extend_from_slice(&input);

Ok(PrecompileOutput::new(0, Bytes::copy_from_slice(&prefix)))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value can be easily fixed in something like the identity precompile where if you would send this input it gets copied to the output and then revm would also pick it up as call options, which would be bad.

The output also impacts the actual return value inside the EVM, so not ideal because can increase gas costs (though maybe not if it's not read, not sure with precompiles), ideally this data is not exposed to the actual EVM.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i know i know
just a temp solution, I'm gonna directly change PrecompileOutput into a enum or something

memory_offset,
call_options: interpreter_result.call_options,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already in result so no separate field necessary?

// env.tx.caller is the Signer of the transaction
// caller is the address of the contract that is calling the precompile
if tx_origin != env.tx.caller.1 || msg_sender != caller.1 {
return Err(Error::XCallOptionsInvalidInputLength.into());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Err(Error::XCallOptionsInvalidInputLength.into());
return Err(Error::XCallOptionsInvalidOrigin.into());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants