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

[Feat] Add network.id and self.address opcodes #27917

Merged
merged 10 commits into from
May 1, 2024
Merged

[Feat] Add network.id and self.address opcodes #27917

merged 10 commits into from
May 1, 2024

Conversation

evan-schott
Copy link
Contributor

Resolves #27912. Additionally, program name references are valid addresses. Example let a: address = hello.aleo.

@evan-schott evan-schott requested a review from d0cd April 23, 2024 21:24
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Attention: Patch coverage is 91.42857% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 76.57%. Comparing base (32aedab) to head (37d0b0e).
Report is 1 commits behind head on mainnet.

Files Patch % Lines
compiler/parser/src/parser/expression.rs 87.50% 2 Missing ⚠️
...mpiler/passes/src/code_generation/visit_program.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           mainnet   #27917      +/-   ##
===========================================
- Coverage    76.59%   76.57%   -0.02%     
===========================================
  Files          208      209       +1     
  Lines         7215     7244      +29     
  Branches      7215     7244      +29     
===========================================
+ Hits          5526     5547      +21     
- Misses        1689     1697       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

The overall approach looks good! Left some comments. Additionally, would you be able to port the network.id execution test from snarkVM over?

compiler/parser/src/tokenizer/token.rs Show resolved Hide resolved
Comment on lines 244 to 258
Expression::Identifier(identifier) if identifier.name == sym::network => match access.name.name {
sym::id => {
// Check that the operation is not invoked outside a `finalize` block.
if self.scope_state.variant != Some(Variant::AsyncFunction) {
self.handler.emit_err(TypeCheckerError::invalid_operation_outside_finalize(
"network.id",
access.name.span(),
))
}
return Some(Type::Integer(IntegerType::U16));
}
_ => {
self.emit_err(TypeCheckerError::invalid_block_access(access.name.span()));
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way we we can de-dup this pattern with the block above? Ditto for the first two blocks.

@@ -273,7 +273,9 @@ impl StatementReconstructor for Flattener<'_> {
let guard = self.construct_guard();

match input.expression {
Expression::Unit(_) | Expression::Identifier(_) => self.returns.push((guard, input)),
Expression::Unit(_) | Expression::Identifier(_) | Expression::Access(_) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are accesses required to be added here? Shouldn't SSA create unique assignment statements for access expressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it because there was a compiler panic when a return statement contained an access expression. Looks like it might not be processed in SSA to ensure proper CG. https://github.com/AleoHQ/leo/blob/e6ba5069dc115837b1df361099db3db265f75676/compiler/passes/src/static_single_assignment/rename_statement.rs#L359

compiler/parser/src/parser/expression.rs Outdated Show resolved Hide resolved
} else {
// Parse instances of `self.address`.
if let Expression::Identifier(id) = expr {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be more "proper" to do the swap in a later pass. That way the parsed AST accurately reflects the input program. A canonicalization pass would be good for this. At the very least, we should add a comment to move this to canonicalization.

@evan-schott
Copy link
Contributor Author

@d0cd Something unexpected about adding the snarkVM test for network.id and block.height is that the height assertion fails for values between 0 and 3. What is the expected block height from the point of view of the test runner?

@evan-schott evan-schott requested a review from d0cd April 29, 2024 23:45
@d0cd
Copy link
Collaborator

d0cd commented Apr 30, 2024

The test runner requires a few blocks initially to set up the test. Updating it to a later number should work.

@evan-schott evan-schott changed the title Feat/add opcodes [Feat] Add network.id and self.address opcodes Apr 30, 2024
Copy link
Collaborator

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!

@d0cd d0cd merged commit 29115ed into mainnet May 1, 2024
15 checks passed
@d0cd d0cd deleted the feat/add-opcodes branch May 1, 2024 13:28
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