Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

Add mypy#116

Closed
ChihChengLiang wants to merge 15 commits into
masterfrom
add-mypy
Closed

Add mypy#116
ChihChengLiang wants to merge 15 commits into
masterfrom
add-mypy

Conversation

@ChihChengLiang
Copy link
Copy Markdown
Collaborator

Fixed #114

@icemelon
Copy link
Copy Markdown
Collaborator

I also suggest changing the line-length to 100. I think 120 is a bit too long.

@ChihChengLiang
Copy link
Copy Markdown
Collaborator Author

@icemelon I created #117 for the line-length change. Feel free to give some reviews or comments :)

Copy link
Copy Markdown
Collaborator Author

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

Hi @han0110 @icemelon,
I have some questions about the instruction part. Not sure what do you think about them.

inputs: Sequence[int],
is_persistent: bool,
) -> Array10:
assert tag.write_only_persistent()
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There's no write_only_persistent, do we want to keep state_write_only_persistent?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, I think we forgot to remove this in previous PR.

Comment thread src/zkevm_specs/evm/instruction.py Outdated
Comment thread src/zkevm_specs/evm/instruction.py Outdated
Comment thread src/zkevm_specs/evm/instruction.py Outdated
rlc_le_bytes = self.rlc_to_le_bytes(rlc)
return self.bytes_to_int(rlc_le_bytes[:n_bytes]), self.is_zero(self.sum(rlc_le_bytes[n_bytes:]))

def rlc_to_int_exact(self, rlc: RLC, n_bytes: int) -> int:
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should the output of rlc_to_int_exact FQ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes

@ChihChengLiang
Copy link
Copy Markdown
Collaborator Author

This task is blocked. We are constantly juggling 3 types:

  • IntEnum
  • FQ
  • RLC

Usually, a cell in a table would be of different types. For example, a value of TxTable could be gasPrice, which is stored in RLC. A value could also be caller_address, which is an FQ.

This makes our txTable lookup outputs Union[RLC, FQ], and we later need to eliminate the type to 1 possibility, which is not Python really good at.

We keep RLC because usually, we want RLC.le_bytes for computation. But the RLC.value is what is acutally stored.

@ChihChengLiang
Copy link
Copy Markdown
Collaborator Author

close in favor of @han0110's new approach. PR would be submitted soon after recent spec PRs are merged.

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Feb 21, 2022

This task is blocked. We are constantly juggling 3 types:

* IntEnum

* FQ

* RLC

Usually, a cell in a table would be of different types. For example, a value of TxTable could be gasPrice, which is stored in RLC. A value could also be caller_address, which is an FQ.

This makes our txTable lookup outputs Union[RLC, FQ], and we later need to eliminate the type to 1 possibility, which is not Python really good at.

We keep RLC because usually, we want RLC.le_bytes for computation. But the RLC.value is what is acutally stored.

This sounds like a case where duck typing is needed, and for the typing we should use something like an Interface. I've taken a look at python support for "Interfaces" and there's Protocol which allows expressing types that must implement some methods.
See https://stackoverflow.com/a/60766965 for an example, and see for the docs https://stackoverflow.com/a/60766965

So we could have something like

class Expression(Protocol):
    def value(self) -> FQ:
        ...

And have IntEnum, FQ and RLC implement the method value

This was referenced Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding mypy checks

4 participants