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

Remove SecondaryAttribute::Event #3885

Open
kevaundray opened this issue Dec 19, 2023 · 2 comments
Open

Remove SecondaryAttribute::Event #3885

kevaundray opened this issue Dec 19, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@kevaundray
Copy link
Contributor

Problem

The enum variant SecondaryAttribute::Event is only used in the aztec_macros, we can remove this and use the is_custom_attribute function

Happy Case

.

Alternatives Considered

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@kevaundray kevaundray added enhancement New feature or request P-LOW labels Dec 19, 2023
@TomAFrench
Copy link
Member

We still have the concept of an event in the main compiler however so it wouldn't be a clean removal.

let events = module
.type_definitions()
.filter_map(|id| {
id.as_type().filter(|struct_id| {
interner
.struct_attributes(struct_id)
.iter()
.any(|attr| attr == &SecondaryAttribute::Event)
})
})
.collect();

@kevaundray
Copy link
Contributor Author

We still have the concept of an event in the main compiler however so it wouldn't be a clean removal.

let events = module
.type_definitions()
.filter_map(|id| {
id.as_type().filter(|struct_id| {
interner
.struct_attributes(struct_id)
.iter()
.any(|attr| attr == &SecondaryAttribute::Event)
})
})
.collect();

Yeah I think we'd need to have the concept of a primary attribute that is able to put things in the abi, ie #[abi(event)]

AztecBot pushed a commit that referenced this issue Jan 8, 2024
This removes the aggregation objects which are currently unused in the
RecursionConstraint implementation. Next we will update the ACVM opcode
to no longer use the aggregation object fields and update the
serialization.

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).

---------

Co-authored-by: kevaundray <kevtheappdev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 📋 Backlog
Development

No branches or pull requests

3 participants