New API #297

siy · 2021-05-25T14:19:30Z

Deep refactoring of the whole subsystem
Endpoint configuration according to new specification
Most endpoints now using JSON RPC protocol
Each endpoint is individually configurable (enable/disable)
Enforced disabling of non-production endpoints (/faucet, /chaos)
Added handling of Radix-specific headers (X-Radixdlt-Method and X-Radixdlt-Correlation-Id)
Restored handling of unstake positions
Added support for configuration of binding address for Node and Archive HTTP servers

iamyulong

Solid work! The new API is well organised and clean.

I only managed to review about 1/3 of it. Will find another time to continue with the remaining.

iamyulong · 2021-06-11T10:16:25Z

radixdlt-core/docs/api-info.md

+
+</details>
+
+## New Organization of APIs


The new organisation looks much better!!!

iamyulong · 2021-06-11T10:25:52Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/controller/HealthController.java

+	@Override
+	public void configureRoutes(final RoutingHandler handler) {
+		handler.get("/health", this::handleHealthRequest);
+		handler.get("/health/", this::handleHealthRequest);


Minor: don't think we need both; it only causes confusion rather than does anything good.

It makes both paths accepted by server. Such a minor thing as trailing slash should not cause issues to client

I understand your intention but not totally agree.

We should encourage clients to use the correct endpoint (less flexible), rather than to correct their mistakes.

iamyulong · 2021-06-11T10:26:32Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/controller/UniverseController.java

-		handler.post("/rpc", this::handleRpc);
-		handler.post("/rpc/", this::handleRpc);
+		handler.get("/universe.json", this::handleUniverseRequest);
+		handler.get("/universe.json/", this::handleUniverseRequest);


radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/data/BalanceEntry.java

radixdlt-core/docs/api-info.md

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/data/TransactionAction.java

iamyulong

A few more comments

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/handler/ArchiveTransactionsHandler.java

iamyulong · 2021-06-11T11:23:54Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/server/NodeHttpServer.java

 		RuntimeProperties properties
 	) {
-		this.port = properties.get("node_api.port", DEFAULT_PORT);
+		this.port = properties.get("api.node.port", DEFAULT_PORT);


We might also consider making the binding network interface configurable, as suggested by #321

It's a good idea, but for other PR

iamyulong · 2021-06-11T11:30:18Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/service/AccountService.java

+	private JSONObject constructBalanceEntry(REAddr rri, UInt384 amount) {
+		return clientApiStore.getTokenDefinition(rri)
+			.fold(
+				__ -> jsonObject().put("rri", "<unknown>").put("amount", amount),


Is it possible to have RRI with no definition?

It's a safety guard in case if there are any issues with DB which may prevent retrieving token definition

talekhinezh · 2021-06-11T16:57:42Z

radixdlt-core/docker/Dockerfile.core

+    RADIXDLT_VALIDATION_API_ENABLE=true \
+    RADIXDLT_UNIVERSE_API_ENABLE=true \
+    RADIXDLT_VERSION_API_ENABLE=true \
+    RADIXDLT_ARCHIVE_API_PORT=8080


These shouldn't all be enabled by default?

talekhinezh · 2021-06-11T16:59:30Z

radixdlt-core/docs/api-info.md

+  - `/validation` - read-only methods which provide same information as available today via all `/node/*` endpoints
+  - `/system` - read-only methods which provide same information as available today via all `/system/*` endpoints
+
+- The `/system`, `/account` and `/validator` endpoints are expected to be protected by firewall and/or require authentication/etc. 


Should be /validation

talekhinezh · 2021-06-11T17:03:11Z

radixdlt-core/docs/api-info.md

+- The `/system`, `/account` and `/validator` endpoints are expected to be protected by firewall and/or require authentication/etc. 
+  (similar requirements/setup as we have today)
+
+### REST APIs


I think we should stop calling these REST endpoints as its not really RESTful: https://en.wikipedia.org/wiki/Representational_state_transfer

talekhinezh · 2021-06-11T17:07:33Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/RadixNodeModule.java

+
+	private UniverseType universeType(UniverseModule universeModule) {
+		try {
+			return universeModule.universe(properties, DefaultSerialization.getInstance()).type();


We shouldn't have getters in Modules as this is the purpose of dependency injection.

This is a workaround for the situation when I need to make configuration decision using already configured in "Guice map" information. Unfortunately, this information is not easily available via other means.

Perhaps we should consider configuring Guice in "layered" fashion, when initially we configure "startup" injector with core things (like runtime properties and universe) and then assemble a working injector using "startup" injector to inject core dependencies.

talekhinezh · 2021-06-11T17:09:40Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/RadixNodeModule.java

+		var nodeEndpoints = enabledNodeEndpoints(properties, universeType);
+
+		bind(new TypeLiteral<List<EndpointConfig>>() {}).annotatedWith(AtNode.class).toInstance(nodeEndpoints);
+		bind(new TypeLiteral<List<EndpointConfig>>() {}).annotatedWith(AtArchive.class).toInstance(archiveEndpoints);


This seems broken to me, we should not have bindings to objects which contain Modules. All information should be communicated via dependency injection.

talekhinezh · 2021-06-11T17:16:29Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/data/TransactionAction.java

+	private final String symbol;
+	private final String iconUrl;
+	private final String tokenUrl;
+	private final String description;


This feels like its starting to become unweildy

Refactored.

talekhinezh · 2021-06-11T17:18:48Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/handler/ArchiveNetworkHandler.java

+import static com.radixdlt.api.JsonRpcUtil.jsonObject;
+import static com.radixdlt.api.JsonRpcUtil.response;
+
+@Singleton


We decided early on not to use @singleton so that this can be managed only by Modules themselves.

Sorry, I was not involved in that decision. Anyway, fixed.

talekhinezh · 2021-06-11T17:22:28Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/qualifier/Account.java

+@Qualifier
+@Target({ FIELD, PARAMETER, METHOD })
+@Retention(RUNTIME)
+public @interface Account {


Account name is too generic and should be changed to something more specific to the API

talekhinezh · 2021-06-11T17:25:14Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/store/ClientApiStore.java

@@ -33,6 +35,7 @@
 /**
 * High level JSON RPC client API store.
 */
+@ImplementedBy(BerkeleyClientApiStore.class)


This should be managed by Modules and not the class itself.

talekhinezh · 2021-06-11T17:25:45Z

radixdlt-core/radixdlt/src/main/java/com/radixdlt/api/service/AccountService.java

+	public AccountService(
+		RadixEngine<LedgerAndBFTProof> radixEngine,
+		@Self ECPublicKey bftKey,
+		ClientApiStore clientApiStore


AccountService should not be dependent on ClientApiStore as we don't want all nodes to run another database

…rometheus

…; Implement single step submission from client

sonarqubecloud · 2021-06-22T14:19:20Z

SonarCloud Quality Gate failed.

4 Bugs
3 Vulnerabilities
1 Security Hotspot
240 Code Smells

52.2% Coverage
1.1% Duplication

siy force-pushed the feature/rpnv1-1306-refactor-json-rpc-implementation branch 3 times, most recently from 564ac24 to 3f8b98c Compare May 31, 2021 09:31

siy force-pushed the feature/rpnv1-1306-refactor-json-rpc-implementation branch 2 times, most recently from b6aeec5 to 01b97cf Compare June 2, 2021 14:17

rvelaz changed the base branch from rc/1.0-beta.35 to rc/1.0-beta.36 June 2, 2021 15:35

siy force-pushed the feature/rpnv1-1306-refactor-json-rpc-implementation branch 7 times, most recently from 018b1c2 to 39e5f76 Compare June 11, 2021 08:10

siy requested review from talekhinezh, LukasGasior1, iamyulong and michalzajda June 11, 2021 09:02

iamyulong reviewed Jun 11, 2021

View reviewed changes

iamyulong approved these changes Jun 11, 2021

View reviewed changes

talekhinezh suggested changes Jun 11, 2021

View reviewed changes

siy force-pushed the feature/rpnv1-1306-refactor-json-rpc-implementation branch from 33bc496 to c4b8efc Compare June 14, 2021 11:17

talekhinezh approved these changes Jun 14, 2021

View reviewed changes

siy force-pushed the feature/rpnv1-1306-refactor-json-rpc-implementation branch 5 times, most recently from 1c34379 to 72ec61b Compare June 21, 2021 07:10

siy added 2 commits June 22, 2021 09:13

Refactor existing JSON-RPC handlers

ccf298f

Intermediate commit

548e591

siy and others added 23 commits June 22, 2021 09:14

Update api-info.md to reflect changes

cf84723

Minor cleanups and preparation to metrics export

4d1ef4a

Do not insert EndpointModule into Guice; Partial metrics implementation

0271528

Refactor TransactionAction

08b9c8a

Add configuration variables

5d657ec

Fix queue flushing and other periodic task handling

6091ddd

Fix non-archive configuration

2404f4f

Cleanups after mergin upstream

48d5afa

Generate all metrics

a963aac

Add space in the header as per the exposition formats mentioned for p…

771e06c

…rometheus

Cleanups and fixes after merging upstream changes; More metrics exported

ed4c4b1

Add CPU and memory metrics

149fb21

Fix parsing/validating RRI

68905f5

Change version string calculation to make it compatible with gradle

8d3d63f

A bunch of minor fixes; Added missing actions to client

63e4834

Fix validator registration test

ace4789

Add remaning tests for async client

071694b

Minor cleanups

9accdfe

Add missing actions

e814af1

Fix rri validation

1cedb34

Minor cleanups

2c79d8d

Add feePayer parameter to build_transaction method

08ddda7

Return real values for validator owners' stake and allowsDelegation flag

a43fe68

siy force-pushed the feature/rpnv1-1306-refactor-json-rpc-implementation branch from 6ec3b96 to a43fe68 Compare June 22, 2021 09:12

siy added 3 commits June 22, 2021 11:35

Update document

cacf8b2

Add health to metrics; Fix parameters for mutable token supply action…

c80b804

…; Implement single step submission from client

Fix default value for allowDelegation flag

ec4ba3f

shambupujar merged commit 4e96308 into rc/1.0-beta.36 Jun 22, 2021

talekhinezh deleted the feature/rpnv1-1306-refactor-json-rpc-implementation branch August 13, 2021 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New API #297

New API #297

siy commented May 25, 2021 •

edited

Loading

iamyulong left a comment

iamyulong Jun 11, 2021

iamyulong Jun 11, 2021

siy Jun 11, 2021

iamyulong Jun 11, 2021

iamyulong Jun 11, 2021

iamyulong left a comment

iamyulong Jun 11, 2021

siy Jun 11, 2021

iamyulong Jun 11, 2021

siy Jun 11, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

talekhinezh Jun 11, 2021

siy Jun 15, 2021

sonarqubecloud bot commented Jun 22, 2021

New API #297

New API #297

Conversation

siy commented May 25, 2021 • edited Loading

iamyulong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamyulong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 22, 2021

siy commented May 25, 2021 •

edited

Loading