Skip to content

chore: enums for statement and op types#69

Merged
arnaucube merged 6 commits into0xPARC:mainfrom
ax0:st-op-enum
Feb 20, 2025
Merged

chore: enums for statement and op types#69
arnaucube merged 6 commits into0xPARC:mainfrom
ax0:st-op-enum

Conversation

@ax0
Copy link
Collaborator

@ax0 ax0 commented Feb 18, 2025

Towards #65.

args.get(1).cloned(),
args.get(2).cloned(),
);
Ok(match (op_code, arg_tup) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we also need to include args.len() in the match to validate the arguments length of each operation

},
// TODO: Remaining ops.
_ => Ok(true)
pub fn check(&self, output_statement: &Statement) -> Result<bool> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function looks much cleaner with this refactor.

src/frontend.rs Outdated
(lt, $($arg:expr),+) => { Operation(NativeOperation::LtFromEntries, args!($($arg),*)) };
(contains, $($arg:expr),+) => { Operation(NativeOperation::ContainsFromEntries, args!($($arg),*)) };
(not_contains, $($arg:expr),+) => { Operation(NativeOperation::NotContainsFromEntries, args!($($arg),*)) };
(eq, $($arg:expr),+) => { Operation::EqualFromEntries($(OperationArg::try_from($arg).unwrap()),*) };
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is cool, now the compiler will complain when using the macro with the wrong number of arguments because it's building an enum.

pub fn args(&self) -> &[OperationArg] {
&self.1
/// Forms operation from op-code and arguments.
pub fn op(op_code: NativeOperation, args: &[Statement]) -> Result<Self> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this method used anywhere?

@ed255
Copy link
Collaborator

ed255 commented Feb 18, 2025

Here's my summary of the new types for Statement and Operation introduced in this PR:

enum frontend::Statement
- fn code(&self) -> middleware::NativeStatement
- fn args(&self) -> Vec<frontend::StatementArg>

frontend::Statement -> middleware::Statement

enum frontend::Operation
- fn code(&self) -> middleware::NativeOperation
- fn args(&self) -> Vec<frontend::StatementArg>

enum middleware::Statement
- fn code(&self) -> middleware::NativeStatement
- fn args(&self) -> Vec<middleware::StatementArg>

enum middleware::Operation
- fn code(&self) -> middleware::NativeOperation
- fn args(&self) -> Vec<middleware::Statement>

// Runtime type-checking 
(middleware::NativeOperation, &[middleware::Statement]) -> middleware::Operation

struct mock_main::Statement(pub middleware::NativeStatement, pub Vec<middleware::StatementArg>)

// Runtime type-checking
mock_main::Statement -> middleware::Statement

middleware::Statement -> mock_main::Statement

And now some thoughts:

  • I think having Statement and Operation defined in the middleware in a strongly-typed is very useful because the type already documents itself on what number of arguments and of which type it uses.
  • I think having runtime type-checkers in the middleware to go from (code, args) -> enum_type is very useful both to be used by frontend and backend. Currently this runtime type-check is implemented for Operation in the middleware and for Statement in mock_main (I would move the later one to the middleware)
  • I'm less convinced about having the Statement and Operation enum types in the frontend, I think they add significant boilerplate and it's not clear to me the advantage of this.

Overall I think there are two important points of discussion for this refactor:

    1. Runtime VS Compile-time Statement/Operation argument type-checking
    1. Ergonomics
    1. Future refactors speed

For (i) I don't see any strong advantage on any direction. Compile-time checking is nice, but I don't think it's super useful for us. I think we can get a nice UX with runtime-checks and proper error reporting. Moreover for custom predicates we can't have compile-time checks; and if we want to unify the API for native predicates and custom predicates we may end up having an API that can't do compile-time checks for native predicates. Although I think it's nice that the backend can receive the statements and arguments already type-checked, so that it has less work to do.

For (ii) I think having the enums in the middleware has benefits because it serves as spec and also allows us to keep the runtime checks in a single place with clean code. On the other hand, I think having the enums in the frontend may worsen the ergonomics.

For (iii) I think this refactor as is would make it more costly to change operations / statements and how their arguments work in the future. For example if we decide to get rid of anchored keys, the current code would make it simpler than the code in this PR. If we decide to allow constants in any statement argument the same reasoning applies. This is another reason why I'm a bit hesitant to have the enum types in the frontend.

}

pub fn tickets_pod_builder(params: &Params, signed_pod: &SignedPod, expected_event_id: i64, expect_consumed: bool, blacklisted_emails: &Value) -> MainPodBuilder {
pub fn tickets_pod_builder(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just a formatting change right? Maybe we should include a rustfmt.toml in the repo to have a single formatting style. @arnaucube

@ed255
Copy link
Collaborator

ed255 commented Feb 19, 2025

looking good!

@ax0 ax0 marked this pull request as ready for review February 19, 2025 12:27
Copy link
Collaborator

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM!

@arnaucube arnaucube added this to the Milestone 2 milestone Feb 19, 2025
@arnaucube arnaucube merged commit c2d23b0 into 0xPARC:main Feb 20, 2025
3 checks passed
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.

3 participants