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: allow users to specify custom contract metadata files #347

Merged
merged 15 commits into from
Nov 26, 2024

Conversation

AlexD10S
Copy link
Collaborator

Allow the user to pass the location of the metadata instead of the path of the contract project. There might be cases where the user only has access to the metadata.

Mentioned in #306 (review)

Copy link
Contributor

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Looks good, think there can be some quick improvements to the code to improve readability and reduce complexity.

As mentioned elsewhere, it seems as though any contract artifact can be used (.contract, .json, .wasm), so perhaps the terminology should be adapted accodingly?

crates/pop-cli/src/commands/call/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/call/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/call/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/call/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/call/contract.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@al3mart al3mart left a comment

Choose a reason for hiding this comment

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

Used this contract to test it out - https://github.com/r0gue-io/faucet-contract

⚙  pop call contract --path ./metadata/faucet.contract --contract 5CwTqjdN28XfcTGsaC5VbyxP8wHAfbaKMwhhGA9bNJjaSDUJ --message drip --url wss://rpc1.paseo.popnetwork.xyz/ --suri //Alice --dry_run
│  
└  Anyhow error: Error invoking `cargo metadata` for ./metadata/faucet.contract/Cargo.toml

It seems that we are failing to resolve paths correctly when a location to an artifact is passed directly instead of pointing to a directory.

Regarding the discussion between metadata or artifact. Knowing that the CLI should work just fine regardless of being pointed to a metadata file or the code or the bundle artifact. Maybe artifact does a better job fitting all scenarios.
We also use "artifacts" wording after a successful contract build.

Copy link
Contributor

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Some suggestions for minor improvements. Also, the .wasm artifact doesnt have any consideration in the tests.

crates/pop-cli/src/commands/call/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/call/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/call/contract.rs Show resolved Hide resolved
crates/pop-cli/src/commands/call/contract.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/common/build.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your patience!

crates/pop-cli/src/commands/call/contract.rs Show resolved Hide resolved
Copy link
Contributor

@al3mart al3mart left a comment

Choose a reason for hiding this comment

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

Quite nice! Thanks.

Now users can provide any contract artifact and perform their calls without problems!

@AlexD10S AlexD10S merged commit 6761f6b into feat-call-ui-contracts Nov 26, 2024
@AlexD10S AlexD10S deleted the chore/metadata-file branch November 26, 2024 16:13
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