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

fix(ICRC_Ledger): FI-1654: Update link to script that downloads latest ICRC ledger #1086

Merged

Conversation

mbjorkqvist
Copy link
Member

The script that downloads the latest ICRC ledger wasm and did files was recently updated to download them using the official ledger releases, rather than the latest commit on the master branch of the ic repository.

The links to the script in the documentation point to a specific commit in the ic repository. This PR proposes to update the links to the commit where the script was updated, so that users end up with an officially released ledger.

@mbjorkqvist mbjorkqvist marked this pull request as ready for review January 24, 2025 10:03
@mbjorkqvist mbjorkqvist requested a review from a team as a code owner January 24, 2025 10:03
Copy link
Member

@marc0olo marc0olo left a comment

Choose a reason for hiding this comment

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

I just want to refer to a recent PR and discussion in that regard where I fixed the link and general and wanted to switch to master for the download script 😅

see #1081

I assume this would have prevented the need for this PR. not sure if there is a really good path to tackle this. maybe there should be a more common place for the script to be stored along with other useful scripts 🤔

@marc0olo marc0olo self-requested a review January 24, 2025 12:26
Copy link
Member

@marc0olo marc0olo left a comment

Choose a reason for hiding this comment

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

@mbjorkqvist just to be sure, the ICP ledger download script did not change, right?

@mbjorkqvist
Copy link
Member Author

mbjorkqvist commented Jan 24, 2025

Thanks for the review and feedback, @marc0olo!

I just want to refer to a recent PR and discussion in that regard where I fixed the link and general and wanted to switch to master for the download script 😅

see #1081

I assume this would have prevented the need for this PR. not sure if there is a really good path to tackle this. maybe there should be a more common place for the script to be stored along with other useful scripts 🤔

Thanks for the pointer! I'd rather point to a specific commit in this case, since there is a non-negligible chance that the files get renamed or moved, in which case the user may have a hard time finding the correct one. In fact, the script would IMO fit better under rs/ledger_suite/scripts than under rs/rosetta-api/scripts, so it may move in the near future, but that would be part of a somewhat larger cleanup item, so I don't want to move it now.

@mbjorkqvist just to be sure, the ICP ledger download script did not change, right?

No, it didn't change (yet). We are planning to create ICP ledger suite releases similar to the ones we now have for the ICRC ledger suite, starting from the next ICP ledger suite version, which is planned for the next week or two, in which case we'd want to update that script also. We have the FI-1647 JIRA ticket for updating the script when the release is done.

@mbjorkqvist mbjorkqvist merged commit 9464f78 into master Jan 24, 2025
11 checks passed
@mbjorkqvist mbjorkqvist deleted the mathias-FI-1654-update-links-to-icrc-download-script branch January 24, 2025 12:38
@letmejustputthishere
Copy link
Member

letmejustputthishere commented Jan 27, 2025

@mbjorkqvist can you please ping us so we can make sure to update the examples READMEs as well. @marc0olo did you update the examples with the current commit hash already, or should we wait for two weeks until FI provides the new scripts?
EDIT: nvm, mixed up the PRs and now see mathias already provided PRs for both portal and examples

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