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: embedding well known canisters at build time #3872

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

NikolaMilosa
Copy link

@NikolaMilosa NikolaMilosa commented Aug 14, 2024

Description

DRE team would highly benefit if we could have a feature like this for well known canisters. Probably others as well. This PR adds a build.rs for dfx-core which creates a file at build time for well known canisters.

Fixes # (issue)

How Has This Been Tested?

This was mostly tested during development with:

./target/debug/dfx --identity default canister --ic call governance list_proposals

# Both following are the same:
./target/debug/dfx canister sign --query rwlgt-iiaaa-aaaaa-aaaaa-cai read --network ic
./target/debug/dfx canister sign --query registry read --network ic

Added two tests to canister sign and canister call scripts

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@NikolaMilosa NikolaMilosa marked this pull request as ready for review August 15, 2024 13:17
@NikolaMilosa NikolaMilosa requested a review from a team as a code owner August 15, 2024 13:17
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity left a comment

Choose a reason for hiding this comment

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

Please add a note to the changelog. This PR should probably also include migration notes, for projects that define these as remote canisters. Migration notes go here: https://github.com/dfinity/sdk/tree/master/docs/migration

Also, I imagine that ideally this kind of change would make it unnecessary to manually define well-known canisters as remote canisters in dfx.json. Does this PR go that far?

src/dfx-core/assets/build.rs Outdated Show resolved Hide resolved
Comment on lines +35 to +49
const WELL_KNOWN_CANISTERS: &[(&str, &str)] = &[
{}
];

pub fn map_wellknown_canisters() -> HashMap<CanisterName, Principal> {{
WELL_KNOWN_CANISTERS.iter().map(|(key, value)| (key.to_string(), (*value).try_into().unwrap())).collect()
}}
",
well_known_canisters
.map(|(key, val)| format!("(\"{}\", \"{}\")", key, val))
.join(",\n")
)
.as_bytes(),
)
.unwrap()

Choose a reason for hiding this comment

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

Since this is a global mapping of well-known canister ids, could we initialize this into a LazyLock rather than adding it as a member to CanisterIdStore?

Copy link
Author

Choose a reason for hiding this comment

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

The version of rust used by sdk doesn't have LazyLock stabilized.

@NikolaMilosa
Copy link
Author

Not sure how I would go about migrations docs for this. Do you have some proposals?

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