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

Update horizon open api to testnet #502

Merged
merged 7 commits into from
Apr 24, 2024
Merged

Conversation

janewang
Copy link
Contributor

This PR attempts to remove the Horizon mainnet url from the open api files and is part of this ticket #478

@janewang janewang requested a review from ElliotFriend April 17, 2024 20:32
@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@stellar-jenkins
Copy link

@ElliotFriend
Copy link
Contributor

this looks good to me! i don't see any problems with it, and main merged pretty nicely right back into it.

Probably the main thing to consider with this are the examples objects. For example, on this horizon page:

On both of those pages, under the "Responses" section, click the RetrieveAnAccountsTransactions button/pill and it pops up with a JSON response. The links in that JSON would point to live data on horizon.stellar.org, but result in 404s on horizon-testnet.stellar.org because the account doesn't exist on tesnet, or the transaction hasn't happened, etc.

Not truly a show-stopper, but could be confusing/weird if someone is looking at those examples, expecting them to provide useable links. I don't know of an alternative way to get predictably 200 status links for testnet horizon (like after a reset, yaknow).

@briwylde08 As the true "docs master," does that bother you any, or is it a non-issue to you?

@janewang
Copy link
Contributor Author

@ElliotFriend This is the same conversation I raised in the channel https://stellarfoundation.slack.com/archives/C049E67BFSP/p1713448837768389

I have been discussing with core to not fully reset and leave some testnet data behind for better developer experience. In the future, I believe testnet should be stable enough for us to be able to show reasonable responses but it is not currently the case. Right now, there's a reset every 3 months. Our options are to show stubbed data or to enable user to input data like from Lab.

Possibly we can mock the response and link to Lab so they could do a proper swagger like request from Lab.

@ElliotFriend
Copy link
Contributor

Aaah, i hadn't seen that conversation. Thanks for pointing it out!

It appears to me that user-editable server URLs are likely not going to be implemented in the openapi plugin we use (see this issue). We could add a server in the config file to act as a stub pretty easily.

Another option might be to use accounts like the Genesis account, or Friendbot account, etc. that we know will be real URLs, that (of course) doesn't cover every possible URL that horizon might return (dex offers, for example). Or, we could have a "setup script" that runs at every testnet reset to create the specific accounts/assets/offers/claimable balances/whatever that we use in our examples.

All that said, I don't think it's a blocker here for this PR.

@janewang
Copy link
Contributor Author

@ElliotFriend Oh I really like the idea of having a script that could run after testnet reset.

I could open a ticket for that. If no blocker for this PR, we could merge. I need a approve to merge.

I have one more section to get to before I finish removing all mainnet horizon from our docs. Looking to get to it next week.

@ElliotFriend
Copy link
Contributor

As I've thought a bit more this afternoon, I may have to walk back a bit on my earlier "not a blocker" statements. Maybe it's a bit rushed to merge it without at least a solid idea of what we will do to alleviate the resulting broken URLs it'll cause.

I can do some thinking this weekend, and come back on Monday with some more thoughts or ideas?

Copy link
Contributor

@ElliotFriend ElliotFriend left a comment

Choose a reason for hiding this comment

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

This is a great change! Thanks

@stellar-jenkins
Copy link

@ElliotFriend ElliotFriend merged commit a4643b4 into main Apr 24, 2024
2 checks passed
@ElliotFriend ElliotFriend deleted the horizon-decentralization-2 branch April 24, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants