[PLT-6847] Payouts new API integration #21

nhenin · 2023-08-24T12:42:26Z

Integration of :

bjornkihlberg

lgtm

hrajchert

This PR changed too many things for me to understand, mainly because I didn't follow the development.

I've made some notes but none of them are blocking.

As an overall note, to improve the likelihood of a feature being used, and now that we have typedoc in place, all PR's should be judged by their documentation.

hrajchert · 2023-08-28T20:55:43Z

packages/runtime/client/rest/src/index.ts

+
+export * from './contract/index.js'
+export * from './withdrawal/index.js'
+export * from './payout/index.js'

 export interface RestAPI {
  healthcheck : () => TE.TaskEither<Error,Boolean>


for the future, the healthcheck returns more than true/false. It also returns the node tip and other useful info.

We need to progressively match 1-on-1 the rest api... this is not the case today... I was thinking taking care of these things when we are going to improve the overall error handling... network mismatch,runtime out of sync etc...

hrajchert · 2023-08-28T20:57:08Z

packages/runtime/client/rest/src/payout/details.ts

+      { payoutId: PayoutId
+      , contractId: ContractId
+      , withdrawalId : optionFromNullable(WithdrawalId) 


Can you add a comment to these? Specially what is the difference between a payoutId and a withdrawalId

yeah it is not clear... I think there is an implicit priority in my head that we need to discuss.. which is external use vs internal concerns... I don't want people to use directly this rest client API, the more I use it the more I'm convinced about that... So I put my efforts first on the interfaces of the packages... This is supposed to be a 1 on 1 of the backend.. if it could be auto-generated, I would do it... I would more add comments on the backend side

hrajchert · 2023-08-28T21:01:21Z

packages/runtime/client/rest/src/payout/endpoints/collection.ts

+export type GETHeadersByRange = (rangeOption: O.Option<ContractsRange>) 
+    => (contractIds : ContractId[]) 
+    => (roles : AssetId[])
+    => (statusOption : O.Option<PayoutStatus>) 
+    => TE.TaskEither<Error | DecodingError,GETByRangeResponse>


Not to change now, but if this is something that a final JS user will use I'd recommend using an object instead of curried arguments.

Also, not sure what this function is supposed to do, adding comments would improve its legibility.

nope they won't use that... I'd like to use curried arguments for internal concerns and uncurried arguments on the user interfaces

hrajchert · 2023-08-28T21:03:59Z

packages/runtime/client/rest/src/payout/endpoints/singleton.ts

+type GETPayload = t.TypeOf<typeof GETPayload>
+const GETPayload = t.type({ links: t.type ({}), resource: PayoutDetails})
+
+export const getViaAxios:(axiosInstance: AxiosInstance) => GET


Not sure what this is supposed to do but the name is a little bit of a code smell.

Should this be called getPayoutDetails? what has Axios have to do with it?

hrajchert · 2023-08-28T21:09:48Z

packages/runtime/client/rest/src/payout/index.ts

+export * from './details.js'
+export * from './header.js'
+export * from './status.js'


This directory is small enough that its contents could live in a single file.

As we talked in a meeting, I recommend watching this talk

I didn't like this talk :-) ...
You see a violation of recipe/ convention and you miss the intent behind...
My Intent here is to get read of infrastructure details and extract the core domain... This is not 100% nailed... I have highlighted that there are concepts of headers and details on all the endpoints and before finding the concepts, I have duplicated logics on each of them...

To be sure the API is consistent

To have a few examples before making it 1 same logic..

High Cohesion / Low Coupling , Clean code , DDD etc... This talk about the life of a file is so rigid... I can't buy it :-) ... When you do little libraries, you can think that this is the case... But when you do something more complex it blows up... Inconsistencies, your dependencies are a mess, you stay monolithic forever etc...

hrajchert · 2023-08-28T21:11:15Z

packages/runtime/client/rest/src/withdrawal/endpoints/collection.ts

@@ -78,10 +70,10 @@ export const PostResponse = t.type({
    });

 export const postViaAxios:(axiosInstance: AxiosInstance) => POST


once again this sounds weird... should this be makeWithdrawl or similar?

it's weird on purpose :-) This is pure mechanical logic... I didn't want to add core domain naming on it

hrajchert · 2023-08-28T21:12:16Z

packages/runtime/client/rest/test/endpoints/contracts.spec.e2e.ts

+import console from "console"
+global.console = console


This seems weird, a little explanation on what and why you did this can go a long way.

yeah Jest console logs doesn't make any sense... I did that to disable it...

hrajchert · 2023-08-28T21:18:12Z

packages/runtime/lifecycle/src/apis/runtimeLifecycle.ts

+      applyInputs : (contractId: ContractId)  => (provideInput : ProvideInput) => TE.TaskEither<Error | DecodingError,ContractId>
+    }
+    payouts :  
+      { available : (filtersOption : O.Option<Filters>) => TE.TaskEither<Error | DecodingError,PayoutAvailable[]>


If this is the end user API, cant we use a available: (filters?: Filters) => Promise<PayoutAvailable[]>?

With typedoc we can use the throws to signal the DecodingError case

cool that was one my questions for a un-fp-ts interface

hrajchert · 2023-08-28T21:22:50Z

packages/wallet/src/nodejs/index.ts

@@ -95,13 +95,13 @@ export class SingleAddressWallet implements WalletAPI {
                                , O.getOrElse(() => 0n))))


-    public tokenBalance : (token : Token) => TE.TaskEither<Error,bigint>
-        = (token) =>
+    public tokenBalance : (assetId : AssetId) => TE.TaskEither<Error,bigint>


I don't understand this change, are you changing Token for Asset in the codebase? I would avoid that as in all the other repos Token is used. Asset is informally the Token+Value combination

Yes, I'm aligning with what Jamie has done in the runtime.... 2 core domains / Marlowe DSL & Runtime 2 different concepts around assets... when we'll do a V2 Marlowe... I have suggested we "uniformize" the naming on the language...

nhenin marked this pull request as draft August 24, 2023 12:46

bjornkihlberg approved these changes Aug 24, 2023

View reviewed changes

nhenin force-pushed the nhenin/payouts branch 2 times, most recently from f923e88 to 5f90854 Compare August 27, 2023 15:57

nhenin changed the title ~~payouts new API integration~~ [PLT-6847] Payouts new API integration Aug 27, 2023

nhenin requested review from bwbush, hrajchert and bjornkihlberg August 27, 2023 15:59

nhenin marked this pull request as ready for review August 27, 2023 16:01

nhenin requested a review from jhbertra August 27, 2023 16:04

hrajchert approved these changes Aug 28, 2023

View reviewed changes

nhenin added 6 commits August 29, 2023 17:46

integrated payout endpoints and new withdraw command

e3b1ef1

[runtime-lifecycle] added payouts logic

4f9488b

[runtime-lifecycle] dissociated Asset model from the rest model

02c33e3

[wallet] added waitConfirmation in Browser Wallet

2e643b8

[rollup] fixed rollup

d6c1746

[versioning] bumped to 0.1.3-beta

e2db0bf

nhenin force-pushed the nhenin/payouts branch from 0d51efe to e2db0bf Compare August 29, 2023 15:49

nhenin merged commit 983f559 into main Aug 29, 2023
1 check failed

nhenin deleted the nhenin/payouts branch August 29, 2023 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLT-6847] Payouts new API integration #21

[PLT-6847] Payouts new API integration #21

nhenin commented Aug 24, 2023 •

edited

bjornkihlberg left a comment

hrajchert left a comment

hrajchert Aug 28, 2023

nhenin Aug 30, 2023

hrajchert Aug 28, 2023

nhenin Aug 30, 2023 •

edited

hrajchert Aug 28, 2023

nhenin Aug 30, 2023 •

edited

hrajchert Aug 28, 2023

hrajchert Aug 28, 2023

nhenin Aug 30, 2023

hrajchert Aug 28, 2023

nhenin Aug 30, 2023 •

edited

hrajchert Aug 28, 2023

nhenin Aug 30, 2023

hrajchert Aug 28, 2023

nhenin Aug 30, 2023

hrajchert Aug 28, 2023

nhenin Aug 30, 2023 •

edited

		@@ -78,10 +70,10 @@ export const PostResponse = t.type({
		});

		export const postViaAxios:(axiosInstance: AxiosInstance) => POST

[PLT-6847] Payouts new API integration #21

[PLT-6847] Payouts new API integration #21

Conversation

nhenin commented Aug 24, 2023 • edited

bjornkihlberg left a comment

Choose a reason for hiding this comment

hrajchert 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

nhenin Aug 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhenin Aug 30, 2023 • edited

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

nhenin Aug 30, 2023 • edited

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

nhenin Aug 30, 2023 • edited

Choose a reason for hiding this comment

nhenin commented Aug 24, 2023 •

edited

nhenin Aug 30, 2023 •

edited

nhenin Aug 30, 2023 •

edited

nhenin Aug 30, 2023 •

edited

nhenin Aug 30, 2023 •

edited