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.
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
ADR 026: Rosetta API Support #6922
ADR 026: Rosetta API Support #6922
Changes from 12 commits
840cdc1
ee79d63
981950c
13b392f
d437a93
9797fbe
06bcefd
77e32d1
06cf545
1fd90c7
1a03d1f
d9b9a9c
163e30c
6787e12
15c6817
1c8e671
d7e654f
cc26e43
217a6ad
2cc9964
b08fe90
bab1c5c
fbd0210
9cb1b28
c28e610
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I would probably add a bit more to the "why?". Rosetta does a pretty good job at this and so you may be able to just take some snippets directly from their page. But essentially anything along the lines of API interop and striving towards a common API standard should suffice.
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.
Historical datasets of what? Previous hub versions? We can probably condense the last two points into one -- support current and previous versions of the Cosmos Hub.
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.
I'm a bit lost. if its a seperate repo why is there an ADR in the sdk repo?
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.
Yeah I'm a bit lost here as well.
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.
The idea was first to be part of the same sdk repo, but @zmanian pointed out that there is one use case where we need to be able to query multiple versions of the hub at the same time, so living in its own repo is more "easy" to maintain than to have the same code in multiple versions of the SDK.
But in the end is very related to the SDK so I opened the ADR here. But if you think is not correct to keep it here we can change it.
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.
This ADR should be in the repo for where the code lives, IMO. What do others think?
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.
I think that is correct, meanwhile because the repo is not created yet (no permissions) we can use this as the draft discussion. Even I am not sure if finally everyone agrees on using another repo, this will be clarified in the cosmos-sdk call today I hope! Thanks @marbar3778
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.
What are
application main binaries
?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.
What's the point in a gateway and the main server? You should describe those, specifically the "why".
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.
Also note from the Common Mistakes:
It's not clear if this means in-process or just the same host, but you should note this. Also, I don't understand what the gateway is, so unless there is a strong reason for it, we should remove this.
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.
Thanks for pointing this out.
I understand both ways are compatible as long as they live in the same Dockerfile (localhost or in-process), but if we had the Rosetta as external process in one Dockerfile and the node in another Dockerfile would not be possible.
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.
There is no mention of how to handle "offline" mode a la multiple modes.
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.
I think a lot is missing from this section, particularly:
Options
andRouter
)Options
and how are they used?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.
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.
In this case we can just create an Adapter of adapter that query different adapters and return the merged response.
Query -> AdapterOfAdapters --> Adapter1 (cosmos-sdk 0.37) (Hub1)
-------------------------------> Adapter2 (cosmos-sdk 0.38) (Hub2)
-------------------------------> Adapter3 (cosmos-sdk 0.38) (Hub3)
The implementation maybe we need to offer for that is one for every SDK and a wrapper adapter of adapters to build complex use cases.
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.