-
Notifications
You must be signed in to change notification settings - Fork 116
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
Solidity verifier initialization #8
Solidity verifier initialization #8
Conversation
…ifier initialization
…g from cbor encoded value
… instead of None. The change was made as previously was assumeed that metadata hash may not exist in the deployed bytecode, while actually it should always exist. Thus, parsing errors indicates that we cannot process further and may need to propagate it further
…y property is required
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.
what do you think about splitting verifier.rs
into several files?
but maybe we don't need it right now, I am not sure
@sevenzing Personally, I prefer to keep it in one file for now. For me, creating a new folder will make it more difficult to navigate between them |
|
||
fn try_from(encoded: bytes::Bytes) -> Result<Self, Self::Error> { | ||
// If metadata is present, last two bytes encode its length in a two-byte big-endian encoding | ||
if encoded.len() < 2 { |
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 suggest adding a struct, which will declaratively describe the bytecode format, i.e.
struct Bytecode {
// bytecode starts with this
compiler: Compiler,
// 4 zeroes specifying the next value
guard: Guard,
// something something size of 16 bytes
foo: Foo,
// last 2 bytes are the length
length: u16,
}
so you could just look at it and understand what format bytecode is, instead of looking at the parse
function and trying to guess something from it (like reading here, that last two bytes are the length etc)
and then implement parse on it (may be DeployedBytecode
already is this struct, but then I'd like to see all the values)
In C, one of the important usages of struct was memory mapping, so you could just convert a pointer to the raw memory (like &[u8]) to this struct and get all the correct values. Time had passed, there is a lot of nuance nowadays (like different endiannesses), but I think the idea in general to "declare" the format for the part of the memory (or bytes) is kinda nice.
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 believe it is an unnecessary work and information right now. as It is an internal structure which is not exposed to outside the module. Current layout allows to effectively and conveniently operate with the structure internally and provide some public api for the users of that structure.
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.
that's not for public interface, but for other developers. With json, for example, you can just look at the json itself to understand the format. With binary formats you can't do that, so it is very helpful if you can see the format "specification" somewhere. Think about yourself after 3 months or about another developer, when you/he'll need to add/fix something connected to the binary format
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's like a comment explaining the format, but more strict
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.
we can do this in another PR tho
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 create an issue to consider that question after PR will be merged
may be create two files without folder? |
What two files you suggest to split it into? Imo, it would be better to leave it as it is now, and think about splitting after the whole functionality would be implemented |
ok, this sounds good to me |
Continues #5
This PR implements verifier initialization. Mostly initialization consists of two main procedures: parsing deployed bytecode and creation transaction input from strings into more structured formats.
The parsing consists of the following steps:
InitializationError::InvalidDeployedBytecode
if the string is invalid hex.solc
key value. If any error occurs at that step,InitializationError::MetadataHashParseError
error is returned (that includes case if byte string was empty).InitializationError::InvalidCreationTxInput
if the string is invalid hex.InitializationError::MetadataHashMismatch
is returned. Otherwise, two parts are extracted from creation transaction input: bytecode located before metadata hash and data located after metadata hash.Currently, the structure responsible for parsing of creation transaction input is named
BytecodeWithConstructorArgs
. The same structure was supposed to be used when parsingLocalBytecode
- a bytecode obtained as a result of local compilation. However, for now constructor args cannot be extracted from the structure. In the next PRs I'll add the constructor arguments into the structure, and add a division indicating whether the structure was obtained from the requester data or from the local compilation result.