-
Notifications
You must be signed in to change notification settings - Fork 206
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
Use stellar-core upgrade config set command #513
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@sisuresh @tsachiherman Looking for your review of this PR. I'm not marking it as ready because I don't want it merged yet because it's still using an unreleased version of stellar-core from @sisuresh's fork. Before merging I'll want to update it to use a patch release of stellar-core. But what I'm looking for review is your thoughts. This PR doesn't leave this in a state that I think is perfect. It's somewhat brittle, although I'd done my best to expose statuses, logs, and check for success as best as possible, without introducing dependencies on more services being up at this point of the upgrade being done. Not entirely proud of it, but I think this is an area we can improve on down the line and I think this is probably just stable enough for dev/testing. |
This change introduces a ~6 second delay into friendbot startup, because friendbot startup is delayed until after network config completes, because network config and friendbot use the same account. I think we'll accept that as an acceptable slow down that'll get better once we can reduce the 4 async steps of config down to 2 via a built-in deployer contract or a built-in config updater contract. cc @dmkozh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think the change is good.
I'd rather seeing us using release core versions rather then private branches.
The PR is temporarily using @sisuresh's PR to validate that it works end to end before we merge and release stellar-core. We need to do more early validation imo. This PR won't be merged until the change in the form is released, which will be hopefully soon in a patch 🙏. |
This change is using stellar-core |
What
Use stellar-core upgrade config set command to set Soroban config instead of using the high limit override.
Why
The high limit override is good, but it is a blunt configuration that only allows setting a single high limit. When it is off the limit is so low it is unusable. There is no way to configure custom limits, or limits that mirror pubnet/testnet/etc.
While this PR doesn't make it configurable, it makes it possible for us to change quickstart's limits, and in a follow up PR we can add options like
--enable-unlimited-limits
or--enable-testnet-limits
. At the moment the PR simply uses testnet limits from a .json file.Close #508