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

Durability bump issue # ODHACK #1639

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

KoxyG
Copy link

@KoxyG KoxyG commented Sep 30, 2024

What

The issue was related to the required durability when bumping/extending a contract. I removed the durability field from the argument struct in the "soroban-cli/command/key" file. As a result, the durability flag will no longer be required when running the command.

Why

This change was made to resolve the issue of requiring the durability flag during contract extension. Additionally, I implemented default persistent storage, so the durability flag is no longer necessary.

Known limitations

There are currently no known limitations.

@KoxyG
Copy link
Author

KoxyG commented Oct 1, 2024

Hi @janewang i saw you reviewed my pr. I also saw one check failed.

Am i missing an installation process?. Also, is there any issue with my pr?

@janewang
Copy link
Contributor

janewang commented Oct 1, 2024

@KoxyG I just kicked off the tests. Some integration tests are failing

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Hi @KoxyG, thanks for contributing.

The durability option needs to be retained. The issue is that it's default value is not functioning.

@willemneal
Copy link
Member

I would change this from clap::ValueEnum to Default, with #[default] above Persistent. Then change the arg to be an option and not use the clap's default value. Then you can use .unwrap_or_default(). TL;DR don't rely on clap for the default value.

#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, clap::ValueEnum)]
pub enum Durability {
/// Persistent
Persistent,
/// Temporary
Temporary,
}

@leighmcculloch
Copy link
Member

@willemneal If the enum is derive Default, then it won't be necessary to make the value an Option<>.

@KoxyG
Copy link
Author

KoxyG commented Oct 2, 2024

Hi @KoxyG, thanks for contributing.

The durability option needs to be retained. The issue is that it's default value is not functioning.

So, should i change it back to the initial state?. The default value is persistent

@KoxyG KoxyG closed this Oct 2, 2024
@KoxyG KoxyG reopened this Oct 2, 2024
@KoxyG
Copy link
Author

KoxyG commented Oct 2, 2024

So, should i change it back to the initial state?. The default storage value is persistent.

The initial issue is that, when the command soroban contract bump --id C... --ledgers-to-expire 1000 is used. It doesn't work unless the --durability persistent is used along side with the command. That was the reason why i remove durability as a compulsory variable in the argument struct @janewang @leighmcculloch @willemneal

@willemneal
Copy link
Member

@leighmcculloch @KoxyG, can either of you reproduce the original issue? I updated the tests to rely on a default durability and it is passing: #1644

@KoxyG
Copy link
Author

KoxyG commented Oct 2, 2024

@leighmcculloch @KoxyG, can either of you reproduce the original issue? I updated the tests to rely on a default durability and it is passing: #1644

You updated the default durability as "persistent" only ?.

@willemneal
Copy link
Member

No just updated the tests to test if the default was working: https://github.com/stellar/stellar-cli/pull/1644/files

So I'm questioning if this PR is needed. If you deploy a contract to testnet and then use extend are you required to use the --durability? When I try it locally and in the test linked above the default seems to be working so perhaps this issue was fixed a while ago. It is from last November so my guess is that the default didn't exist then and was added without closing that issue.

@KoxyG
Copy link
Author

KoxyG commented Oct 29, 2024

Hi @janewang I noticed you reviewed my pull request. Do i need to work on anything froom my end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

soroban-cli: unnecessarily requires durability when bumping a contract
4 participants