all: TSS RPC Caller Service #40

gouthamp-stellar · 2024-09-15T22:49:13Z

What

This is TSS's RPC Caller Service as described in this doc:
https://docs.google.com/document/d/1xcX86-w8lwT_60flCuYBK9X9co-108-5X6D9e5epw0U/edit#heading=h.76emp0zcbvdy

Why

This service is responsible for handling incoming transaction submission requests from clients and calling RPC (for the very first time) to submit those transactions to the network

Known limitations

N/A

Issue that this PR addresses

https://app.asana.com/0/1207947010297920/1208138733594316

Checklist

PR Structure

It is not possible to break this PR down into smaller PRs.
This PR does not mix refactoring changes with feature changes.
This PR's title starts with name of package that is most changed in the PR, or all if the changes are broad or impact many packages.

Thoroughness

This PR adds tests for the new functionality or fixes.
All updated queries have been tested (refer to this check if the data set returned by the updated query is expected to be same as the original one).

Release

This is not a breaking change.
This is ready to be tested in development.
The new functionality is gated with a feature flag if this is not ready for production.

adding semicolons

…ransaction_hash ...also remove outgoing/incoming_status and have one status column

commit fb807aa Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Tue Sep 3 10:28:49 2024 -0700 remove the index on try_transaction_xdr and add column/index on try_transaction_hash ...also remove outgoing/incoming_status and have one status column commit 6fc0dc2 Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Tue Sep 3 08:58:46 2024 -0700 missing , commit a9cf4e3 Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Tue Sep 3 01:55:23 2024 -0700 make hash primary key instead of xdr commit c0f9d32 Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Fri Aug 30 15:18:27 2024 -0700 moving all migrations to one file commit 2de9898 Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Fri Aug 30 15:16:53 2024 -0700 adding semicolons adding semicolons commit 373c71a Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Fri Aug 30 15:12:24 2024 -0700 update commit 3f9f9f0 Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Fri Aug 30 15:12:00 2024 -0700 remove empty lines commit 9920f48 Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Fri Aug 30 15:06:40 2024 -0700 TSS tables and the channel interface commit a58d519 Author: gouthamp-stellar <goutham.patnaikuni@stellar.org> Date: Fri Aug 30 10:55:22 2024 -0700 tables and interfaces for TSS

…and builds a transaction out of it

…he Payload

gouthamp-stellar · 2024-09-23T20:19:38Z

internal/tss/channels/utils.go

+	if err != nil {
+		return tss.RPCSendTxResponse{}, fmt.Errorf("%s: Unable to upsert try in tries table: %s", channelName, err.Error())
+	}
+	if rpcErr != nil && rpcSendResp.Code.OtherCodes == tss.RPCFailCode || rpcSendResp.Code.OtherCodes == tss.UnMarshalBinaryCode {


should this be done before the 1st UpsertTry?

We need to update the try with the rpcfailcode or unmarshallerrror code, which is why we call UpsertTry first before checking to see if there was an error

gouthamp-stellar · 2024-09-23T20:41:58Z

internal/tss/services/rpc_caller_service.go

@@ -0,0 +1,21 @@
+package services
+
+import (


Maybe make channels and services one struct

Done. Removed services, now only have channels and the router

daniel-burghardt · 2024-09-27T20:04:07Z

internal/entities/rpc.go

+type RPCEntry struct {
+	Key                   string `json:"key"`
+	XDR                   string `json:"xdr"`
+	LastModifiedLedgerSeq int64  `json:"lastModifiedLedgerSeq"`
+}


These won't be used anymore, we can clean them up. I had deleted them from the RPC Service PR.

Yeap, removed these so you can add them later

internal/serve/serve.go

daniel-burghardt · 2024-09-27T20:14:59Z

internal/serve/serve.go

+	tssChannelConfigs := tsschannel.RPCCallerChannelConfigs{
+		TxManager:     txManager,
+		Store:         store,
+		MaxBufferSize: cfg.RPCCallerServiceChannelBufferSize,
+		MaxWorkers:    cfg.RPCCallerServiceChannelMaxWorkers,
+	}
+	rpcCallerServiceChannel := tsschannel.NewRPCCallerChannel(tssChannelConfigs)


Shall we initialize all services the same way for better readability? Same for the NewTransactionService above.

Suggested change

tssChannelConfigs := tsschannel.RPCCallerChannelConfigs{

TxManager: txManager,

Store: store,

MaxBufferSize: cfg.RPCCallerServiceChannelBufferSize,

MaxWorkers: cfg.RPCCallerServiceChannelMaxWorkers,

}

rpcCallerServiceChannel := tsschannel.NewRPCCallerChannel(tssChannelConfigs)

rpcCallerServiceChannel := tsschannel.NewRPCCallerChannel(tsschannel.RPCCallerChannelConfigs{

TxManager: txManager,

Store: store,

MaxBufferSize: cfg.RPCCallerServiceChannelBufferSize,

MaxWorkers: cfg.RPCCallerServiceChannelMaxWorkers,

})

Yeap, these are all being initialized the same now

internal/tss/channels/rpc_caller_channel.go

daniel-burghardt · 2024-09-27T20:39:42Z

internal/tss/router/router.go

+func (r *router) Route(payload tss.Payload) error {
+	var channel tss.Channel
+	if payload.RpcSubmitTxResponse.Status.Status() != "" {
+		switch payload.RpcSubmitTxResponse.Status.Status() {
+		case string(tss.NewStatus):
+			channel = r.RPCCallerChannel
+		case string(entities.TryAgainLaterStatus):
+			channel = r.ErrorJitterChannel
+		case string(entities.ErrorStatus):
+			if payload.RpcSubmitTxResponse.Code.OtherCodes == tss.NoCode {
+				if slices.Contains(JitterErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {
+					channel = r.ErrorJitterChannel
+				} else if slices.Contains(NonJitterErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {
+					channel = r.ErrorNonJitterChannel
+				} else if slices.Contains(FinalErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {
+					channel = r.WebhookChannel
+				}
+			}
+		default:
+			// Do nothing for PENDING / DUPLICATE statuses
+			return nil
+		}
+	}
+	if channel == nil {
+		return fmt.Errorf("payload could not be routed - channel is nil")
+	}
+	channel.Send(payload)
+	return nil
+}


💡 No problem with the logic, just an idea for improved readability:

Suggested change

func (r *router) Route(payload tss.Payload) error {

var channel tss.Channel

if payload.RpcSubmitTxResponse.Status.Status() != "" {

switch payload.RpcSubmitTxResponse.Status.Status() {

case string(tss.NewStatus):

channel = r.RPCCallerChannel

case string(entities.TryAgainLaterStatus):

channel = r.ErrorJitterChannel

case string(entities.ErrorStatus):

if payload.RpcSubmitTxResponse.Code.OtherCodes == tss.NoCode {

if slices.Contains(JitterErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {

channel = r.ErrorJitterChannel

} else if slices.Contains(NonJitterErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {

channel = r.ErrorNonJitterChannel

} else if slices.Contains(FinalErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {

channel = r.WebhookChannel

}

}

default:

// Do nothing for PENDING / DUPLICATE statuses

return nil

}

}

if channel == nil {

return fmt.Errorf("payload could not be routed - channel is nil")

}

channel.Send(payload)

return nil

}

func (r *router) Route(payload tss.Payload) error {

switch payload.RpcSubmitTxResponse.Status.Status() {

case string(tss.NewStatus):

r.RPCCallerChannel.Send(payload)

case string(entities.TryAgainLaterStatus):

r.ErrorJitterChannel.Send(payload)

case string(entities.ErrorStatus):

if payload.RpcSubmitTxResponse.Code.OtherCodes == tss.NoCode {

if slices.Contains(JitterErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {

r.ErrorJitterChannel.Send(payload)

} else if slices.Contains(NonJitterErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {

r.ErrorNonJitterChannel.Send(payload)

} else if slices.Contains(FinalErrorCodes, payload.RpcSubmitTxResponse.Code.TxResultCode) {

r.WebhookChannel.Send(payload)

} else {

return fmt.Errorf("payload could not be routed, status %+v", payload.RpcGetIngestTxResponse.Status)

}

}

// Do nothing for PENDING / DUPLICATE statuses

case string(entities.PendingStatus), string(entities.DuplicateStatus):

return nil

default:

return fmt.Errorf("unexpected status %+v", payload.RpcGetIngestTxResponse.Status)

}

return nil

}

That is more readable, but the thing is, I want to be able to pass nil values for some/all channels. Because with the ingestor for example, it will only ever need the webhook channel. And if im passing nil values for these, I think I should explicitly check to see if the channels are nil before sending payloads to them

internal/tss/services/transaction_manager.go

daniel-burghardt · 2024-09-27T20:48:54Z

internal/tss/services/transaction_manager.go

+	err = t.Store.UpsertTry(ctx, payload.TransactionHash, feeBumpTxHash, feeBumpTxXDR, rpcSendResp.Code)
+	if err != nil {
+		return tss.RPCSendTxResponse{}, fmt.Errorf("%s: Unable to upsert try in tries table: %s", channelName, err.Error())
+	}


The UpsertTry should likely be called after checking the previous errors, right?

No, because I want UpsertTry to capture the error code returned by SendTransaction

internal/tss/store/store.go

daniel-burghardt

Just two small new comments: about the %e and the parseErr != nil check

daniel-burghardt · 2024-10-02T19:33:49Z

internal/tss/services/transaction_manager.go

@@ -64,7 +64,7 @@ func (t *transactionManager) BuildAndSubmitTransaction(ctx context.Context, chan
 		return rpcSendResp, fmt.Errorf("%s: RPC fail: %w", channelName, parseErr)
 	}

-	if rpcErr != nil && rpcSendResp.Code.OtherCodes == tss.RPCFailCode || rpcSendResp.Code.OtherCodes == tss.UnmarshalBinaryCode {
+	if parseErr != nil && rpcSendResp.Code.OtherCodes == tss.RPCFailCode || rpcSendResp.Code.OtherCodes == tss.UnmarshalBinaryCode {


This case is impossible atm, isn't it? If parseErr != nil it would fall into the previous check.

gouthamp-stellar added 30 commits August 30, 2024 10:55

tables and interfaces for TSS

a58d519

TSS tables and the channel interface

9920f48

remove empty lines

3f9f9f0

update

373c71a

adding semicolons

2de9898

adding semicolons

moving all migrations to one file

c0f9d32

make hash primary key instead of xdr

a9cf4e3

missing ,

6fc0dc2

remove the index on try_transaction_xdr and add column/index on try_t…

fb807aa

…ransaction_hash ...also remove outgoing/incoming_status and have one status column

commit #2

d774f2a

changing from RpcIngestTxResponse -> RpcGetIngestTxResponse

6b04d62

Merge branch 'tss_tables_interfaces' into transaction_module

f2fefef

latest changes

b441a31

add tests for ValidateOptions

02c7de3

changes based on comments

f9a5611

Merge branch 'main' into tss_tables_interfaces

32bd6ce

Merge branch 'tss_tables_interfaces' into transaction_module

5c7db27

latest changes based on changes to interface

c3e8de9

string -> RPCTXStatus

82b12fb

Merge branch 'tss_tables_interfaces' into transaction_module

9a9c9be

adding a transaction_builder.go which takes a list of operation xdrs …

0662da9

…and builds a transaction out of it

moving transaction_service to the utils dir

8f76d61

upper case Channel methods

456b71f

Merge branch 'tss_tables_interfaces' into transaction_module

7974aa9

latest changes

5bdf8f2

p.txService.NetworkPassPhrase()

3890bfe

last commit before writing unit tests

e87c6d6

Making the transaction service more injectible and adding fields to t…

c1d3786

…he Payload

typo

01ed3eb

gouthamp-stellar added 2 commits September 21, 2024 19:00

casing

db8714d

better test for Send

2c0d060

gouthamp-stellar changed the base branch from main to transaction_module September 23, 2024 00:29

gouthamp-stellar changed the base branch from transaction_module to main September 23, 2024 00:38

gouthamp-stellar changed the base branch from main to transaction_module September 23, 2024 00:38

Merge branch 'transaction_module' into rpc_caller_service

b82a25a

gouthamp-stellar commented Sep 23, 2024

View reviewed changes

Base automatically changed from transaction_module to main September 25, 2024 03:40

gouthamp-stellar added 11 commits September 24, 2024 21:48

Merge branch 'main' into rpc_caller_service

101bd43

incorporating Daniel's changes + comments

637aeaf

delete files

0fb94e3

remove unused code

df7ae05

name change

cf2dc18

remove commented code

16581fd

moving the mocks file inside servicesmocks dir

ab7c0d7

name changes

5fe5d0b

Merge branch 'main' into rpc_caller_service

78042c9

merging main

9118a06

fixing parsesendresp tests

12425f9

daniel-burghardt reviewed Sep 27, 2024

View reviewed changes

gouthamp-stellar added 3 commits September 30, 2024 15:42

changes based on comments

6b2a6f7

removing test case that is not relevant anymore

b4f6052

making sure the pool implements the channel interface

01ad88d

daniel-burghardt approved these changes Oct 2, 2024

View reviewed changes

gouthamp-stellar added 3 commits October 8, 2024 19:11

Merge branch 'main' into rpc_caller_service

26e8733

adding tracker_dsn and other env vars

33c2b78

go mod tidy

3cdacaa

gouthamp-stellar merged commit dd833aa into main Oct 9, 2024
5 checks passed

gouthamp-stellar deleted the rpc_caller_service branch October 9, 2024 02:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: TSS RPC Caller Service #40

all: TSS RPC Caller Service #40

gouthamp-stellar commented Sep 15, 2024 •

edited

Loading

gouthamp-stellar Sep 23, 2024

gouthamp-stellar Sep 27, 2024

gouthamp-stellar Sep 23, 2024

gouthamp-stellar Sep 27, 2024

daniel-burghardt Sep 27, 2024

gouthamp-stellar Oct 1, 2024

daniel-burghardt Sep 27, 2024

gouthamp-stellar Oct 1, 2024

daniel-burghardt Sep 27, 2024

gouthamp-stellar Oct 1, 2024

daniel-burghardt Sep 27, 2024

gouthamp-stellar Oct 1, 2024

daniel-burghardt left a comment

daniel-burghardt Oct 2, 2024

all: TSS RPC Caller Service #40

all: TSS RPC Caller Service #40

Conversation

gouthamp-stellar commented Sep 15, 2024 • edited Loading

What

Why

Known limitations

Issue that this PR addresses

Checklist

PR Structure

Thoroughness

Release

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

daniel-burghardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouthamp-stellar commented Sep 15, 2024 •

edited

Loading