Conversation
|
@zixuanzh CI is failing because you need to run |
| type Pop = Vec<ValueType>; | ||
| type Push = Vec<ValueType>; | ||
|
|
||
| struct Signature { |
There was a problem hiding this comment.
Can you document these structs with triple-slash comments? That way rustdoc can automatically generate a docs site for this.
There was a problem hiding this comment.
@zixuanzh @Steampunkery please document the code.
| push: Push | ||
| } | ||
|
|
||
| pub enum Filter { |
There was a problem hiding this comment.
Perhaps it would be useful to have per-instruction granularity here?
There was a problem hiding this comment.
Per instruction granularity would be a tricky/impossible thing to implement because you need to simulate an entire stack to know if the right data types will be on that stack at the right time. If you mean just simulating a stack and filtering out certain instructions, then yes, I could do that.
| for signature_value in &signature.pop { | ||
| let value = self.stack.pop(); | ||
| match value { | ||
| Some(stack_value) => { |
There was a problem hiding this comment.
It is a matter of preference but I find it cleaner, in the case of destructuring two-valued enums like Option, to use the if let Some(stack_value) pattern here.
|
@zixuanzh CI seems to be a version behind on edit: actually that doesn't seem to be the problem. trying to find out why fmt is still freaking out. edit 2: Are you sure you are using the correct version? |
zixuanzh
force-pushed
the
verify-instructions
branch
from
c451588 to
ad30d14
Compare
|
I am running on 1.32.0-nightly, fmt fails at deployer.rs etc in CI, it passes CI when I only commit verify-instructions related files. |
|
@zixuanzh is this ready or WIP? Please add the respective label. Also please rebase. |
zixuanzh
force-pushed
the
verify-instructions
branch
from
ad30d14 to
93e2c7f
Compare
|
@jakelang it should be ready as it is unless @Steampunkery has something to add, we don't have access to add labels. |
| } | ||
|
|
||
| pub trait InstructionValidator { | ||
| fn validate(&mut self, module: &Module) -> Result<bool, InstructionError>; |
There was a problem hiding this comment.
I think since we have ModuleError, it should be possible to merge the InstructionError enum into it and thus removing the need for a new trait.
|
I think this looks good in general, great work @Steampunkery and @zixuanzh! |
| InvalidOperation(Instruction), | ||
| } | ||
|
|
||
| impl fmt::Display for InstructionError { |
There was a problem hiding this comment.
If we merge ModuleError and InstructionError, could you create a new PR which just adds the Display and Error traits on ModuleError? Then this PR can just extend that with the new enum values.
There was a problem hiding this comment.
Alternatively, instead of adding new variants, it may make sense to just encode these errors as a string in the Custom variant of ModuleError.
There was a problem hiding this comment.
Or make Custom(T) type generic and throw InstructionError in there.
There was a problem hiding this comment.
Text in general is fine, but for programmatic detection the enums are better.
There was a problem hiding this comment.
I'd like to stick to enum variants if possible, and I can certainly look into merging InstructionError into ModuleError.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
|
@Steampunkery @zixuanzh can we take over this PR or do you think you'll have some resources to work on this in the next while? |
|
@axic please proceed, we are unlikely to work on this in the immediate future. thanks |
jakelang
added
deferred
axic
force-pushed
the
verify-instructions
branch
from
93e2c7f to
df99108
Compare
4 participants
As per issue #18
parity-wasm checks types for const operators but not for binary as specified in the spec.
We simulated the stack to check for types and hence we added a new validator interface that takes in a mutable self reference. We also use a self defined enum instead of String for error handling and propagation as advised by the documentation.