feat: effective target canister ID for mgmt call #954
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
krpeacock
requested changes
Nov 26, 2024
|
Sure. I added a line to the CHANGELOG in 5ea9b5b. |
krpeacock
approved these changes
Nov 26, 2024
peterpeterparker
added a commit
to dfinity/ic-js
that referenced
this pull request
Nov 27, 2024
# Motivation It’s literally impossible to test the internal `transform` function used within the static `create` function of the `ic-mgmt` class. However, as we need to address an issue to support WASM installation with chunks (#773), we must extend it. That’s why this PR extracts the function into a module, allowing us to provide tests for the mapping. # Notes We delegate the responsibility of calling the `transform` function to `agent-js`, as it is used internally there and as we are "just" implementing the interface exposed by agent-js. While it is not thoroughly covered at the moment in the agent, I submitted a PR [#954](dfinity/agent-js#954) today that adds at least one assertion. No particular entry in CHANGELOG. This is an internal refactoring. # Changes - Extract internal `transform` function to a utility. - JSdocs and tests. - Improve types of the function (expected interface and return type) --------- Signed-off-by: David Dal Busco <david.dalbusco@dfinity.org> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Similarly to ic-js (see PR #773), the goal of this PR is to extend the
transformfunction of thegetManagementCanisterto support calls on the mainnet toinstall_chunked_code. While I have not tested the function of agent-js irl, I can confirm that without extending the similar function inic-js, I am unable to perform such calls on the mainnet.This issue is due to the following specification:
See documentation
Notes
Because I am not familiar with the agent-js codebase and want to avoid introducing too many changes, this PR solely focuses on adding the necessary code for the intended functionality. There are likely ways to improve the type declarations and address some code duplication after this implementation, but I did not want to add unnecessary noise.
There is also a potential "race condition" if a
transformfunction is called with bothcanister_idandtarget_canister. In such cases,target_canisterwill take precedence overcanister_id. For the same reasons mentioned above, I did not want to add additional complexity to the implementation, as this was not the primary goal of the PR. However, it is likely an issue worth addressing in the future.Similarly, this PR includes a test specifically for the
target_canistermapping toeffective_canister_id, which duplicates some existing mocking. It would likely be beneficial to reduce code duplication in this area. Additionally, it would be useful to add tests for the other mappings ofgetManagementCanister, but this is beyond the scope of this feature.