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

Support combining actors and returning imports #431

Closed
wants to merge 2 commits into from

Conversation

ncpenke
Copy link

@ncpenke ncpenke commented May 24, 2023

Overview
Why do we need this feature? What are we trying to accomplish?

Two use cases:

  • Allow modular candid files to define canister interfaces in a model where a canister can implement different interfaces
  • Expose imports to facilitate implementation of custom downstream code generators

Requirements
What requirements are necessary to consider this problem solved?

  • A mechanism exists to combine actors across imported files
  • A mechanism exists to detect imported did file changes for code generation

Considered Solutions
What solutions were considered?

Re-implement outside of candid, but the broader community wont' benefit from this

Recommended Solution
What solution are you recommending? Why?

A new check_file_with_options API that allows passing custom options. This is extensible in the future for other use-cases. The option will allow actors to be combined in certain cases.

Considerations
What impact will this change have on security, performance, users (e.g. breaking changes) or the team (e.g. maintenance costs)?

Minimal impact. This change includes a performance bug fix. The newer functionality is implemented in a backwards compatible manner.

@ghost ghost added the cla:agreed label May 24, 2023
@chenyan-dfinity
Copy link
Contributor

Can you elaborate on the use case a bit more? Why is import not enough?

@ncpenke
Copy link
Author

ncpenke commented May 25, 2023

Can you elaborate on the use case a bit more? Why is import not enough?

import allows importing the types, but not the service.

In our use-case we have interfaces which define both the types, and service methods associated with those types. For example we have a backup interface which lets us take a backup of the canister data, a logger interface which lets us query the canister's logs. These interfaces consist of both the types, and the service methods (both update and query calls) that define the interface. We were having to manually copy/paste the methods in the final canisters that were using them, instead it would be preferable to maintain these interfaces separately and modularly include them in the final canister.

The other use-case is exposing the imports set. We have a custom rust agent that we generate off the parsed data (since the rust agent that's currently in progress wasn't read before). In order to update the auto-generated code we need to know if any of the imported files changed.

@ncpenke ncpenke force-pushed the master branch 2 times, most recently from 55f1f37 to 9c7df5c Compare December 8, 2023 20:15
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This mostly looks good. My only concern is on the spec side. The spec didn't clearly specify the behavior when the imported did file contains a main service. I will try to clarify the spec tomorrow. Then we can merge this PR.

@ncpenke
Copy link
Author

ncpenke commented Dec 12, 2023

Awesome, thank you!

@chenyan-dfinity
Copy link
Contributor

Closed in favor of #507

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants