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

refactor: read function names from contract classes #10753

Merged
merged 5 commits into from
Dec 18, 2024

Conversation

alexghr
Copy link
Contributor

@alexghr alexghr commented Dec 16, 2024

This PR removes contract artifacts from the archiver as they were only used for dubugging purposes. Function names are now (optionally) emitted when registereing contract classes.

I have created #10752 to remove function names from the contract class once we don't need this debug information

@alexghr alexghr force-pushed the ag/archiver-no-artifacts branch from 894542d to cf05dc7 Compare December 16, 2024 09:54
Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Change looks good, but note that the function name never makes it to the node. I'm fine removing the artifact to reduce memory usage, but note that this will mean we won't have function names available.

What we need to do to support that is convert the function selectors into function names on the client, where we do have the artifact.

@alexghr alexghr force-pushed the ag/archiver-no-artifacts branch from d202708 to 72a9a67 Compare December 16, 2024 16:28
@alexghr
Copy link
Contributor Author

alexghr commented Dec 16, 2024

Refactored it so the archiver keeps a mapping from selector->function name rather than using the class registration event

yarn-project/archiver/src/factory.ts Show resolved Hide resolved
yarn-project/circuit-types/src/interfaces/archiver.test.ts Outdated Show resolved Hide resolved
// TODO: Node should validate the artifact before accepting it
return this.contractDataSource.addContractArtifact(address, artifact);
public registerContractFunctionNames(_address: AztecAddress, names: Record<string, string>): Promise<void> {
return this.contractDataSource.registerContractFunctionNames(names);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having the caller pass the selector and name, I think it'd be best if they pass the full function signature, so the server computes the selector from it and extracts the name. This way we don't risk a malicious user overwriting valid selectors with trash.

And I'm fine pushing this for later, so feel free to create an issue instead of addressing now.

@alexghr alexghr force-pushed the ag/archiver-no-artifacts branch from ede179d to 32246b3 Compare December 18, 2024 15:21
@alexghr alexghr enabled auto-merge (squash) December 18, 2024 15:28
@alexghr alexghr merged commit 921febb into master Dec 18, 2024
71 checks passed
@alexghr alexghr deleted the ag/archiver-no-artifacts branch December 18, 2024 16:07
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