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: Use full IC Management IDL #406

Merged
merged 7 commits into from
Mar 24, 2022

Conversation

megrogan
Copy link
Contributor

@megrogan megrogan commented May 6, 2021

No description provided.

@megrogan megrogan requested review from hansl and krpeacock May 6, 2021 13:47
@krpeacock
Copy link
Contributor

@megrogan I'd like to approve this - could you clean up the merge conflicts?

# Conflicts:
#	packages/agent/src/canisters/management.ts
#	packages/agent/src/canisters/management_idl.ts
@megrogan
Copy link
Contributor Author

megrogan commented Nov 4, 2021

@krpeacock I have pushed 2 commits to my branch but seems like there is a github issue because the PR has not been updated. I will create a new PR.

@megrogan
Copy link
Contributor Author

megrogan commented Nov 4, 2021

I think this was because my fork was still private whereas this repo has since become public. I have now made my fork public but the PR has not updated yet...

@megrogan
Copy link
Contributor Author

megrogan commented Nov 4, 2021

This is my issue:
https://stackoverflow.com/questions/36225381/reattach-a-detached-fork-in-github

I am following the 2nd answer and contacting github support...

@megrogan
Copy link
Contributor Author

megrogan commented Nov 5, 2021

They have re-attached my fork

@krpeacock
Copy link
Contributor

What a weird state to get into!

@megrogan
Copy link
Contributor Author

megrogan commented Nov 10, 2021

I just tried pushing another commit (just removed a full-stop from a comment!) and that seems to have finally wired it back up such that the other commits since my original one have now been applied to the PR too!

@megrogan
Copy link
Contributor Author

Argh! Failing e2e tests. Will investigate

@hansl
Copy link
Contributor

hansl commented Mar 23, 2022

@krpeacock Can we get this closed? I don't think this is being worked on.

@krpeacock
Copy link
Contributor

@megrogan I can't update your PR, but making this change to label the method as an ActorMethod will get the final test passing

https://github.com/dfinity/agent-js/blob/kyle/ic-management/e2e/node/basic/basic.test.ts#L50

@krpeacock
Copy link
Contributor

That does raise a separate issue that we ought to figure out how to get ActorMethod typing to automatically apply to actors instantiated with generated types

@krpeacock krpeacock merged commit 9b663c6 into dfinity:main Mar 24, 2022
@megrogan megrogan deleted the full-management-IDL branch March 24, 2022 15:04
ericswanson-dfinity pushed a commit that referenced this pull request Apr 4, 2022
* feat: Use full IC Management IDL

* Assert fn prov...with_cycles has type ActorMethod

Co-authored-by: Kyle Peacock <kylpeacock@gmail.com>
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