-
Notifications
You must be signed in to change notification settings - Fork 115
Conversation
# Conflicts: # src/main/scala/scorex/core/api/http/PeersApiRoute.scala
# Conflicts: # src/main/resources/reference.conf # src/main/scala/scorex/core/api/http/NodeViewApiRoute.scala
Changes Unknown when pulling 434275e on swagger-api into ** on master**. |
Changes Unknown when pulling 5059062 on swagger-api into ** on master**. |
Changes Unknown when pulling 1aa80b0 on swagger-api into ** on master**. |
Changes Unknown when pulling ec9a2e6 on swagger-api into ** on master**. |
|
||
/peers/connect: | ||
post: | ||
summary: Get current size of the unconfirmed transactions pool |
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.
Is this summary correct?
"com.github.mpilquist" % "simulacrum_2.12" % "0.10.0", | ||
"com.github.swagger-akka-http" % "swagger-akka-http_2.12" % "0.10.0", | ||
"com.google.guava" % "guava" % "20.0", | ||
"com.google.guava" % "guava" % "19.0", |
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.
Why go down from 20.0 to 19.0?
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.
Library, that required guava 20 was removed, we now only have libraries that require guava 19 in dependencies
@@ -15,25 +15,32 @@ trait ApiRoute extends Directives { | |||
val context: ActorRefFactory | |||
val route: Route | |||
|
|||
implicit val timeout = Timeout(settings.timeout) | |||
implicit lazy val timeout: Timeout = Timeout(settings.timeout) |
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.
Is this timeout
still used? Maybe this line could be removed?
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.
Or is it required as an implicit argument somewhere?
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.
It is required in a lot of API routes as a timeout for requests to actors. Check ApiRouteWithFullView
for example
Changes Unknown when pulling 3b60395 on swagger-api into ** on master**. |
Changes Unknown when pulling 5379653 on swagger-api into ** on master**. |
Changes Unknown when pulling 29d8205 on swagger-api into ** on master**. |
# Conflicts: # src/main/scala/scorex/core/api/http/PeersApiRoute.scala # src/main/scala/scorex/core/api/http/swagger/SwaggerDocService.scala
@Tolsi , the CI build is failing. |
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 noticed that all @Path
, @ApiOperation
and @ApiImplicitParams
annotations were removed.
Do I understand correctly that goal is to document the API in "yaml" files (e.g. examples/src/main/resources/api/testApi.yaml
) from now on, to conform with newer openapi conventions?
Some of the information that we had in annotations has not been added to any yaml file (yet). The examples/src/main/resources/api/testApi.yaml
, for instance, contains documentation only for a few paths. Many paths that were documented with annotations before would have no documentation if this PR were accepted in its current state. There would be a loss of documentation coverage. Would that be ok? Shouldn't we try to maintain and increase our documentation coverage level?
|
||
/peers/connect: | ||
post: | ||
summary: Get current size of the unconfirmed transactions pool |
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.
Is this the correct summary for this path?
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.
@ceilican I think nope. I fixed this one here https://github.com/ergoplatform/ergo/pull/108/files#diff-f11bc37099101a6156775fbde1871902R712
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.
Your fix seems good to me.
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.
@ceilican But it was done in ergo repository =)
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.
Yes, I know. I think it should be done here (in Scorex, in this PR) too.
And I wonder if it would be possible/desirable, in the future (in another PR), to reduce code and documentation duplication at the api level between scorex('s example) and ergo. It seems to me that right now duplication is only avoided at the network layer. All other layers remain completely abstract in Scorex's core and need to be (re-)implemented, possibly in similar ways, by every blockchain system built with Scorex. Perhaps, in the future, Scorex could provide some standard api routes/paths already pre-implemented.
@Daron666, is ergo's api significantly different from the api of scorex's example?
@kushti, should we include the api into the scope of that architecture review that we have discussed?
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.
@ceilican Ergo's api is a superset over scorex api.
…ger-api Conflicts: examples/src/main/scala/examples/hybrid/api/http/StatsApiRoute.scala src/main/scala/scorex/core/api/http/PeersApiRoute.scala src/main/scala/scorex/core/api/http/UtilsApiRoute.scala
@ceilican review pleasse |
No description provided.