Report right protocol version in horizon root path #783
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
charlie-wasp
force-pushed
the
issue-58-wrong-protocol-version
branch
from
e1305ce
to
ac60d66
Compare
|
@charlie-wasp thanks for this! Can you bring your branch up to date with master? That'll fix the build issue this PR is hitting. |
charlie-wasp
force-pushed
the
issue-58-wrong-protocol-version
branch
from
ac60d66
to
f6ea0bf
Compare
|
@tomquisel done! ✅ |
|
Because of the concerns in stellar/stellar-core#1740 I think we can try to make it by introducing new fields with unambiguous names and deprecating existing
Then in the next minor release (we should really start following semver...) we will remove |
|
@bartekn 👌 I'll give it a shot |
charlie-wasp
force-pushed
the
issue-58-wrong-protocol-version
branch
2 times, most recently
from
919fad8
to
f85e4c6
Compare
|
@bartekn @tomquisel I added new fields and updated the branch. And seemingly nothing is broken 😅 |
bartekn
suggested changes
Jan 25, 2019
services/horizon/internal/app.go
Outdated
| redis *redis.Pool | ||
| coreVersion string | ||
| horizonVersion string | ||
| protocolVersion int32 // will be deprecated in the next minor release |
There was a problem hiding this comment.
It's not part of the public API, can be removed right away.
| @@ -18,6 +18,7 @@ func PopulateRoot( | |||
| hVersion, cVersion string, | |||
| passphrase string, | |||
| pVersion int32, | |||
| coreProtocolVersion int32, | |||
There was a problem hiding this comment.
Let's change these names to currentProtocolVersion and coreSupportedProtocolVersion. Will be less confusing.
protocols/horizon/main.go
Outdated
| HistoryElderSequence int32 `json:"history_elder_ledger"` | ||
| CoreSequence int32 `json:"core_latest_ledger"` | ||
| NetworkPassphrase string `json:"network_passphrase"` | ||
| ProtocolVersion int32 `json:"protocol_version"` // will be deprecated in the next minor release |
There was a problem hiding this comment.
Could you change this to:
// Deprecated - remove in: horizon-v0.17.0
ProtocolVersion int32 `json:"protocol_version"` ProtocolVersionto support #833. This will help us automatically detect deprecations.
charlie-wasp
force-pushed
the
issue-58-wrong-protocol-version
branch
from
6806aa5
to
2d57fb0
Compare
|
@bartekn done! ✅ Thank you for the review! |
bartekn
force-pushed
the
horizon-0.16.0
branch
from
66feb30
to
0d43840
Compare
bartekn
force-pushed
the
issue-58-wrong-protocol-version
branch
from
979f966
to
e47de18
Compare
bartekn
approved these changes
Jan 28, 2019
|
LGTM! Added small changes in e47de18. |
This was referenced Jan 30, 2019
oryband
added a commit
to kinecosystem/go
that referenced
this pull request
Feb 17, 2019
* tag 'horizon-v0.16.0': horizon: 0.16.0 CHANGELOG (stellar#851) Allow smooth migration for failed transactions column (stellar#847) Clean up the redundancy in the Horizon command line option handling (stellar#834) horizon/actions: fix a bug in `GetPageQuery` and `GetAddress` (stellar#846) remove JSON horizon/actions: check hash before sending an event to the stream of orderbook (stellar#828) Deprecated check (stellar#838) Report right protocol version in horizon root path (stellar#783) fix migration ordering (stellar#821) Add failed transactions to ledger resource (stellar#756) Ignore "0" trades effects (stellar#791) Add package index to README (stellar#831) horizon/db2/history: optimize account queries for payments and operations (stellar#829) Update "gap detected" message (stellar#830) Update setTickInProgress comment (stellar#824) Bump postgres version for Travis (stellar#822) fix wrong effect of a circular path payment (stellar#807) Update README.md (stellar#820) Fix build badge (stellar#816) Require migration to run Horizon version successfully (fixes stellar#778) (stellar#808)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #580
As stated in stellar/stellar-core#1740, I take here current protocol version from the
ledger.versionattribute of stellar-core info command responseI am a quite newbie in both Stellar and Go, so I will appreciate any comments, which help me improve this PR 🙏