stellar / stellar-cli

CLI for Stellar developers
68 stars 68 forks source link

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

Open leighmcculloch opened 11 months ago

leighmcculloch commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

Bump was renamed to extend in:

JoE11-y commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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

https://github.com/stellar/stellar-cli/pull/1639

KoxyG commented 6 days ago

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

https://github.com/stellar/stellar-cli/pull/1639#issuecomment-2391896434