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 __get_candid_interface_tmp_hack endpoint #4386

Merged
merged 5 commits into from
Feb 7, 2024
Merged

Conversation

chenyan-dfinity
Copy link
Contributor

Now that candid are in the canister metadata for quite a while, tmp_hack is redundant. It also cannot be set to private like we do in metadata.

Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great! Can you add to the Changelog.md?

@ggreif ggreif changed the title remove __get_candid_interface_tmp_hack endpoint remove __get_candid_interface_tmp_hack endpoint Feb 6, 2024
Copy link

github-actions bot commented Feb 7, 2024

Comparing from fae8386 to 0f8a87d:
In terms of gas, 2 tests regressed, 1 tests improved and the mean change is +1.4%.
In terms of size, 5 tests improved and the mean change is -0.4%.

@chenyan-dfinity
Copy link
Contributor Author

@ggreif Why does it cause gas change at all?

@crusso
Copy link
Contributor

crusso commented Feb 7, 2024

@ggreif Why does it cause gas change at all?

I think this is due to the static heap being different which can affect deserialization of bignums due to @gabor's speculative decoding of compact bignums. @luc-blaeser look familiar to you?

I kicked off the mac build again since it failed due to drun non-determinism.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

LGTM, but I wonder if any external folk are relying on it....

Thanks for cleaning this up!

@chenyan-dfinity chenyan-dfinity added the automerge-squash When ready, merge (using squash) label Feb 7, 2024
@mergify mergify bot merged commit 7e2421f into master Feb 7, 2024
10 checks passed
@mergify mergify bot deleted the remove-tmp-hack branch February 7, 2024 19:34
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Feb 7, 2024
@crusso
Copy link
Contributor

crusso commented Feb 7, 2024

Hmm, I'm not sure why the perf changed, tbh. I think my original hunch was wrong, actually. There's no deserialization going on here, is there?

@chenyan-dfinity
Copy link
Contributor Author

The benchmark may contain deserialization, but I don't know why the perf changed.

@rdobrik
Copy link

rdobrik commented Mar 6, 2024

We are still using it in our Java Agent so we need to fix it and replace with metadata. Any example how to get idl from running canister using metadata?

@chenyan-dfinity
Copy link
Contributor Author

chenyan-dfinity commented Mar 6, 2024

If you already have an agent library, you can fetch the state tree, e.g., read_state, with path /canister/<canister_id>/metadata/candid:service. See the spec: https://internetcomputer.org/docs/current/references/ic-interface-spec/#state-tree-canister-information.

Concretely, here is the code in Rust: https://github.com/dfinity/agent-rs/blob/main/ic-agent/src/agent/mod.rs#L994 and code in agent-js: https://github.com/dfinity/agent-js/blob/main/packages/agent/src/canisterStatus/index.ts#L144

@rdobrik
Copy link

rdobrik commented Mar 6, 2024

If you already have an agent library, you can fetch the state tree, e.g., read_state, with path /canister/<canister_id>/metadata/candid:service. See the spec: https://internetcomputer.org/docs/current/references/ic-interface-spec/#state-tree-canister-information.

Concretely, here is the code in Rust: https://github.com/dfinity/agent-rs/blob/main/ic-agent/src/agent/mod.rs#L994 and code in agent-js: https://github.com/dfinity/agent-js/blob/main/packages/agent/src/canisterStatus/index.ts#L144

Thank you, I will fix our Java code.

@rdobrik
Copy link

rdobrik commented Mar 8, 2024

Worked. Fixed our Java Agent. We will publish fixed library ASAP. Thanks for help.

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.

4 participants