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(iota-move): fix os error 2 for iota move new command using <NAME> with uppercase letter(s) #4916

Conversation

roman1e2f5p8s
Copy link
Contributor

@roman1e2f5p8s roman1e2f5p8s commented Jan 20, 2025

Description of change

  • Use the provided <NAME> instead of its lowercase variant to obtain the correct and existing path for writing.
  • Update <NAME> validation error message: since <NAME> is validated as an Identifier, a valid <NAME> can also start with an uppercase letter or underscore.

Links to any relevant issues

Fixes #4895.

Type of change

  • Bug fix (a non-breaking change which fixes an issue)

How the change has been tested

(EDIT: On Linux) Run the following before and after the change:

cargo build
./target/debug/iota move new Xxx

Change checklist

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that new and existing unit tests pass locally with my changes

@roman1e2f5p8s roman1e2f5p8s added sc-platform Issues related to the Smart Contract Platform group. dev-tools Issues related to the Developer Tools Team core-protocol labels Jan 20, 2025
@roman1e2f5p8s roman1e2f5p8s self-assigned this Jan 20, 2025
@roman1e2f5p8s roman1e2f5p8s requested review from a team as code owners January 20, 2025 10:17
Copy link

vercel bot commented Jan 20, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
apps-backend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2025 3:45pm
apps-ui-kit ✅ Ready (Inspect) Visit Preview Jan 31, 2025 3:45pm
rebased-explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2025 3:45pm
wallet-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2025 3:45pm

Copy link
Member

@lzpap lzpap left a comment

Choose a reason for hiding this comment

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

Is there any other place in the stack where package names are assumed to be lowercase? @valeriyr
Afaik the on-chain packages are always denoted via their IDs, so the package name is simply a client side concept right now, correct?

@valeriyr
Copy link
Contributor

valeriyr commented Jan 22, 2025

Is there any other place in the stack where package names are assumed to be lowercase? @valeriyr Afaik the on-chain packages are always denoted via their IDs, so the package name is simply a client-side concept right now, correct?

It looks like the issue can be reproduced only on Linux. Yesterday I tried it locally without changes from this PR on macOS and the result was the following:

./target/debug/iota move new Xxx

tree ./Xxx
./Xxx
├── Move.toml
├── sources
│   └── xxx.move
└── tests
    └── xxx_tests.move

3 directories, 3 files

Xxx is used only like a folder name and as the package name is the toml file name = "Xxx". In external smart contracts, a package is accessible using a named address which is xxx = "0x0" in the example. I think we need to check the changes from this PR carefully before merging.

@valeriyr
Copy link
Contributor

In a Move.lock file we also can find a package name as a reference:

[[move.package]]
name = "Iota"
source = { git = "https://github.com/iotaledger/iota.git", rev = "testnet", subdir = "crates/iota-framework/packages/iota-framework" }

Iota is capitalized, btw.

@roman1e2f5p8s
Copy link
Contributor Author

@valeriyr, yes, I discovered the issue and tested the changes on Linux, I didn't have the chance to check it on macOS. Nevertheless, I am surprised how and why the issue is not reproduced on macOS: when using ./target/debug/iota move new Xxx, the following line constructs path as ./xxx instead of ./Xxx:

let p = path.unwrap_or_else(|| Path::new(&name));

However, ./xxx might not exist, which results in attempts to write to a non-existing path:
let mut w = std::fs::File::create(
p.join(SourcePackageLayout::Sources.path())
.join(format!("{name}.move")),
)?;
writeln!(
w,
r#"/*
/// Module: {name}
module {name}::{name} {{
}}
*/"#,
name = name
)?;

Also, this

self.new
.execute(path, [(IOTA_PKG_NAME, IOTA_PKG_PATH)], [(name, "0x0")], "")?;

creates and writes to ./Xxx (I think that's correct), but everything below that will attempt to write to ./xxx.

@alexsporn
Copy link
Member

FYI: MacOS is by default case-insensitive

@roman1e2f5p8s
Copy link
Contributor Author

Thanks, @alexsporn, that explains why the issue is not reproducible on macOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-protocol dev-tools Issues related to the Developer Tools Team research sc-platform Issues related to the Smart Contract Platform group.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug (iota-move)]: iota move new fails if using only the required argument <NAME> with uppercase letter(s)
6 participants