-
Notifications
You must be signed in to change notification settings - Fork 5
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: configure CMC to play nice with cycles ledger #72
Conversation
Co-authored-by: Eric Swanson <[email protected]>
canister_name: "cycles_ledger", | ||
canister_id: "um5iw-rqaaa-aaaaq-qaaba-cai", | ||
wasm_name: "cycles_ledger.wasm.gz", | ||
wasm_url: "https://github.com/dfinity/cycles-ledger/releases/latest/download/cycles-ledger.wasm.gz", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes builds and tests non-reproducible, if they depend on this extension. It's already a problem that this is the case for II and nns-dapp, above. Do we need to have the same issue with the cycles-ledger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've asked that this issue with II and nns-dapp be corrected in #60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. No, this is not needed, and we can make the update script keep this up to date if we ever want the extension to install the cylces ledger
This reverts commit 5a66c6f.
extensions/nns/src/install_nns.rs
Outdated
.unwrap(), | ||
) | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the unwrap on Encode!() I can go along with, .write_all().unwrap() probably not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and there were more mistakes. I must have dropped the self-review somehow
No description provided.