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

soroban-cli: unnecessarily requires durability when bumping a contract #1078

Closed
leighmcculloch opened this issue Nov 11, 2023 · 8 comments · May be fixed by #1639
Closed

soroban-cli: unnecessarily requires durability when bumping a contract #1078

leighmcculloch opened this issue Nov 11, 2023 · 8 comments · May be fixed by #1639
Assignees
Labels
bug Something isn't working

Comments

@leighmcculloch
Copy link
Member

What version are you using?

$ soroban version
soroban 20.0.0-rc.4.2 (v20.0.0-rc.4.1-4-g4e9a48756b96e4e1a070105bad202f39901139ad-dirty)
soroban-env 20.0.0-rc2 (8c63bff68a15d79aca3a705ee6916a68db57b7e6)
soroban-env interface version 85899345977
stellar-xdr 20.0.0-rc1 (d5ce0c9e7aa83461773a6e81662067f35d39e4c1)
xdr curr (9ac02641139e6717924fdad716f6e958d0168491)

What did you do?

$ soroban contract bump --id C... --ledgers-to-expire 1000

What did you expect to see?

Successful bump of contract instance.

What did you see instead?

Error message saying that durability must be specified.

error: the following required arguments were not provided:
  --durability <DURABILITY>

Usage: soroban contract bump --ledgers-to-expire <LEDGERS_TO_EXPIRE> --durability <DURABILITY> --rpc-url <RPC_URL> --network-passphrase <NETWORK_PASSPHRASE> --id <CONTRACT_ID>

Discussion

The contract bump subcommand when executed with only a contract ID and no storage key, will bump the contract instance. The contract instance is always stored in persistent storage, so there's no need to require that durability be specified.

Additionally, if someone does accidentally specify temporary, the error message is not trivial to understand. So the application can lead a developer down a path that is confusing.

❯ soroban contract bump --id C... --ledgers-to-expire 1000 --durability temporary
error: transaction simulation failed: cannot compute bump rent changes

Caused by:
    0: cannot find bump footprint ledger entry with key ContractData(LedgerKeyContractData { contract: Contract(Hash(c9add98fd70da6af658f4b569d5766a65111b5793b6c1c124d2192a81b346206)), key: LedgerKeyContractInstance, durability: Temporary })
    1: not found
@leighmcculloch leighmcculloch added the bug Something isn't working label Nov 11, 2023
@leighmcculloch
Copy link
Member Author

The same problem exists when bumping a WASM. WASM is only ever stored in persistent storage. The CLI requires durability to be specified. It actually allows temporary to be specified and ignores it and bumps the persistent.

@stellarsaur
Copy link
Contributor

With the latest changes on main, it looks like we do not support the bump command anymore. What would be the equivalent functionality, and are we still experiencing this issue?

./target/debug/soroban contract --help
Tools for smart contract developers

Usage: soroban contract <COMMAND>

Commands:
  bindings  Generate code client bindings for a contract
  build     Build a contract from source
  extend    Extend the time to live ledger of a contract-data ledger entry
  deploy    Deploy a contract
  fetch     Fetch a contract's Wasm binary
  inspect   Inspect a WASM file listing contract functions, meta, etc
  install   Install a WASM file to the ledger without creating a contract instance
  invoke    Invoke a contract function
  optimize  Optimize a WASM file
  read      Print the current value of a contract-data ledger entry
  restore   Restore an evicted value for a contract-data legder entry

For reference, my CLI version:

./target/debug/soroban version
soroban 20.0.0-rc4 (v20.0.0-rc4-42-g7f8ad60b9921b43871b2a5ec7e4d9494b909446f)
soroban-env 20.0.0-rc2 (2674d867d7c6aa4212abab05ff30e5804ff1db90)
soroban-env interface version 85899345989
stellar-xdr 20.0.0-rc1 (9c97e4fa909a0b6455547a4f4a95800696b2a69a)
xdr curr (6a620d160aab22609c982d54578ff6a63bfcdc01)

@leighmcculloch
Copy link
Member Author

Bump was renamed to extend in:

@JoE11-y
Copy link

JoE11-y commented Sep 26, 2024

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

With over four years in blockchain and backend development, I’ve worked across different ecosystems, handling everything from smart contract design to on-chain interactions and protocol integration. I focus on building secure, scalable, and reliable blockchain applications, managing both on-chain and off-chain infrastructure.

How I plan on tackling this issue

I'd review the logic causing the issue, explore alternative solutions, and wrap up with thorough testing to ensure the fix works across different scenarios

@KoxyG
Copy link

KoxyG commented Sep 26, 2024

Hi @leighmcculloch and @janewang I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

My name is Koxy. I'm a blockchain Rust developer and Stellar ecosystem contributor. As an SCF Pathfinder and contributor to Stellar documentation, I have hands-on experience with both Rust and the Stellar platform.

My background in open-source development and familiarity with Stellar's architecture positions me well to address the timing issues in Soroban CLI tests.

I'm eager to implement immediate solutions while collaborating with the team on long-term improvements to enhance Soroban CLI's reliability.

How I plan on tackling this issue

To approach this problem with the Soroban CLI's contract bump command, I would:

  1. Investigate: First, I'd review the current implementation of the contract bump command, focusing on how it handles durability and storage.

  2. Confirm Behavior: Verify the issue by reproducing it in different environments to ensure it's consistent.

  3. Analyze Requirements: Determine if durability is actually necessary for bumping contract instances, given that they're always stored in persistent storage.

  4. Propose Changes: Modify the CLI code to:
    a) Remove the mandatory durability flag for contract instance bumps.
    b) Default to persistent storage for contract instances.
    c) Improve error messaging if temporary storage is accidentally specified.

  5. Implement: Make the necessary code changes, ensuring backward compatibility if possible.

  6. Test Thoroughly: Create comprehensive tests covering various scenarios, including edge cases.

  7. Update Documentation: Revise CLI documentation and help messages to reflect the changes.

  8. Submit PR: Create a pull request with detailed explanations of the changes and their rationale.

  9. Collaborate: Work with maintainers to address any feedback and iterate on the solution as needed.

@Luluameh
Copy link

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I have a solid background in JavaScript and TypeScript development, particularly in building and maintaining command-line interfaces. My experience includes debugging and resolving issues related to contract interactions in blockchain environments.

How I plan on tackling this issue

I would analyze the bump command implementation to understand why the durability requirement is enforced. After confirming that a contract instance is always stored in persistent storage, I’d modify the code to remove the durability requirement. Additionally, I would enhance error handling to provide clearer messages for accidental temporary durability specifications.

@KoxyG
Copy link

KoxyG commented Sep 30, 2024

Hi @janewang @stellarsaur I just worked on this issue. Kindly help review. This is my pull request.

#1639

@janewang janewang linked a pull request Oct 1, 2024 that will close this issue
@janewang janewang added this to DevX Oct 1, 2024
@github-project-automation github-project-automation bot moved this to Backlog (Not Ready) in DevX Oct 1, 2024
@janewang janewang moved this from Backlog (Not Ready) to Needs Review in DevX Oct 1, 2024
@KoxyG
Copy link

KoxyG commented Oct 5, 2024

@ElliotFriend pls help review this as well. This is an issue I worked on from ODhack.

#1639 (comment)

@janewang janewang removed the ODHack8 label Oct 29, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in DevX Oct 29, 2024
@janewang janewang removed the status in DevX Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

6 participants