feat: sidecar query server #6953

p0mvn · 2023-11-30T03:22:09Z

Closes: #XXX

What is the purpose of the change

Core implementation of the sidecar query server with the router

Separated from: #6785

SQS is not wired into the app yet. I suggest review being only cursory. Please check go.mod, that there is no state-machine affecting logic.

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

czarcas7ic

ACK on skim, noting that I did not look at tests. Full feature will need proper review

czarcas7ic · 2023-11-30T19:02:45Z

ingest/sqs/README.md

+- How to handle atomicity between ticks and pools? E.g. let's say a block is written between the time initial pools are read
+and the time the ticks are read. Now, we have data that is partially up-to-date.


Can/should we just operate on data from the previous block?

czarcas7ic · 2023-11-30T19:09:11Z

ingest/sqs/chain_info/ingester/redis/chain_info_ingester.go

+// NewChainInfoIngester returns a new chain information ingester.
+func NewChainInfoIngester(chainInfoRepo mvc.ChainInfoRepository, repositoryManager mvc.TxManager) mvc.AtomicIngester {
+	return &chainInfoIngester{
+		chainInfoRepo:     chainInfoRepo,
+		repositoryManager: repositoryManager,
+	}
+}


Is there a reason we don't just set the logger directly here?

czarcas7ic · 2023-11-30T19:09:56Z

ingest/sqs/chain_info/ingester/redis/chain_info_ingester.go

+func (ci *chainInfoIngester) ProcessBlock(ctx sdk.Context, tx mvc.Tx) error {
+	height := ctx.BlockHeight()
+
+	ci.logger.Info("ingesting latest blockchain height", zap.Int64("height", height))


I don't think this should be an info log, we should keep info logs as bare as needed, especially for something that will happen on a per block basis.

czarcas7ic · 2023-11-30T19:13:18Z

ingest/sqs/domain/mocks/pool_mock.go

+// GetTotalValueLockedUOSMO implements domain.PoolI.
+func (mp *MockRoutablePool) GetTotalValueLockedUOSMO() math.Int {
+	return mp.TotalValueLockedUSDC
+}
+


Method is UOSMO be returns USDC

czarcas7ic · 2023-11-30T19:13:45Z

ingest/sqs/domain/mocks/pool_mock.go

+	newDenoms := make([]string, len(mp.Denoms))
+	copy(newDenoms, mp.Denoms)
+
+	newTotalValueLocker := osmomath.NewIntFromBigInt(mp.TotalValueLockedUSDC.BigInt())


Locker instead of Locked

czarcas7ic · 2023-11-30T19:51:29Z

ingest/sqs/pools/ingester/redis/taker_fee.go

+func retrieveTakerFeeToMapIfNotExists(ctx sdk.Context, denoms []string, denomPairToTakerFeeMap domain.TakerFeeMap, poolManagerKeeper common.PoolManagerKeeper) error {
+	for i, denomI := range denoms {
+		for j := i + 1; j < len(denoms); j++ {
+			if !denomPairToTakerFeeMap.Has(denomI, denoms[j]) {
+				takerFee, err := poolManagerKeeper.GetTradingPairTakerFee(ctx, denomI, denoms[j])
+				if err != nil {
+					// This error should not happen. As a result, we do not skip it
+					return err
+				}
+
+				denomPairToTakerFeeMap.SetTakerFee(denomI, denoms[j], takerFee)
+			}
+		}
+	}
+	return nil
+}


Oh I see, we set it here. Can probably save on storage by making it so that if no store exists we use default rather than storing default for ever non-overriden pair.

czarcas7ic · 2023-11-30T19:54:21Z

ingest/sqs/pools/usecase/pools_usecase.go

+// NewPoolsUsecase will create a new pools use case object
+func NewPoolsUsecase(timeout time.Duration, poolsRepository mvc.PoolsRepository, redisRepositoryManager mvc.TxManager) mvc.PoolsUsecase {
+	return &poolsUseCase{
+		contextTimeout:         timeout,
+		poolsRepository:        poolsRepository,
+		redisRepositoryManager: redisRepositoryManager,
+	}
+}


Is there a better name for usecase? I am still struggling to understand how usecase applies here

czarcas7ic · 2023-11-30T19:54:48Z

ingest/sqs/pools/usecase/pools_usecase.go

+		return nil, err
+	}
+
+	// TODO: refactor get these directl from the pools repository.


Suggested change

// TODO: refactor get these directl from the pools repository.

// TODO: refactor get these directly from the pools repository.

czarcas7ic · 2023-11-30T19:57:40Z

ingest/sqs/router/delivery/http/router_handler.go

+// Define a regular expression pattern to match sdk.Coin where the first part is the amount and second is the denom name
+// Patterns tested:
+// 500ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2
+// 100uion
+var coinPattern = regexp.MustCompile(`([0-9]+)(([a-z]+)(\/([A-Z0-9]+))*)`)


regex will prob need to consider multiple slash for token factory, it might already do this but I am skimming

czarcas7ic · 2023-11-30T19:58:58Z

ingest/sqs/router/repository/redis/redis_router_repository.go

+}
+
+const (
+	keySeparator = "-"


I am skimming but is it possible for a denom to have "-" now in sdk 47 so this would maybe mess up the key

p0mvn · 2023-11-30T21:34:54Z

Thanks for the review. I reviewed all comments, and all are non-blocking. I will come back to addressing after the v1 release.

Tracking that here: https://app.clickup.com/t/86a1j92hy

* feat(SQS): IngestManager abstraction and wiring * comment * comment * lint * feat: sidecar query server --------- Co-authored-by: Adam Tucker <adamleetucker@outlook.com> (cherry picked from commit 4edf2cb) # Conflicts: # go.mod # go.sum

* feat(SQS): IngestManager abstraction and wiring * comment * comment * lint * feat: sidecar query server --------- Co-authored-by: Adam Tucker <adamleetucker@outlook.com> (cherry picked from commit 4edf2cb)

* feat(SQS): IngestManager abstraction and wiring * comment * comment * lint * feat: sidecar query server --------- Co-authored-by: Adam Tucker <adamleetucker@outlook.com> (cherry picked from commit 4edf2cb) Co-authored-by: Roman <roman@osmosis.team>

Co-authored-by: roman <roman@osmosis.team>

mattverse · 2023-12-01T08:14:23Z

ingest/sqs/domain/mocks/pool_mock.go

+}
+
+// SetTokenOutDenom implements domain.RoutablePool.
+func (*MockRoutablePool) SetTokenOutDenom(tokenOutDenom string) {


what's the reason we're not implementing this for mock?

mattverse · 2023-12-01T08:25:06Z

ingest/sqs/domain/mvc/doc.go

@@ -0,0 +1,3 @@
+// Encapsulates the Model-View-Controller abstraction domain.


just curious, what was the original intention of this file:?

mattverse · 2023-12-01T09:20:35Z

ingest/sqs/domain/mvc/repository.go

+
+// AtomicIngester is an interface that defines the methods for the atomic ingester.
+// It processes a block by writing data into a transaction.
+// The caller must call Exec on the transaction to flush data to sink.


what does this mean?

mattverse · 2023-12-01T09:38:43Z

ingest/sqs/domain/router.go

+	MaxRoutes          int
+	MaxSplitRoutes     int
+	MaxSplitIterations int
+	// Denominated in OSMO (not uosmo)


just curious, why do we want this in osmo not uosmo?

mattverse · 2023-12-01T09:41:49Z

ingest/sqs/log/logger.go

+	Debug(msg string, fields ...zap.Field)
+}
+
+type NoOpLogger struct{}


Why do we want no op logger?

mattverse · 2023-12-01T09:43:14Z

ingest/sqs/log/logger.go

+			return nil, err
+		}
+
+		core = zapcore.NewTee(


While testing, I found a bug that logger wouldnt correctly pipe it to std out besides in the ingester. Needs investigation

mattverse · 2023-12-01T09:45:52Z

ingest/sqs/domain/pools.go

+	sqsModel := p.GetSQSPoolModel()
+	poolDenoms := p.GetPoolDenoms()
+
+	if len(poolDenoms) < 2 {


Why is this?

mattverse · 2023-12-01T09:50:17Z

ingest/sqs/pools/ingester/redis/pool_ingester.go

+// updateRoutes updates the routes for all denom pairs in the taker fee map. The taker fee map value is unused.
+// It returns a channel that is closed when all routes are updated.
+// TODO: test
+func (pi *poolIngester) updateRoutes(ctx context.Context, tx mvc.Tx, pools []domain.PoolI, denomPairToTakerFeeMap map[domain.DenomPair]osmomath.Dec) chan domain.DenomPair {


just curious, why are we using channels here?

p0mvn added 4 commits November 29, 2023 23:53

feat(SQS): IngestManager abstraction and wiring

5690de4

comment

05c4a56

comment

79265e3

lint

a780bdf

github-actions bot added the T:build label Nov 30, 2023

feat: sidecar query server

c576065

p0mvn force-pushed the roman/sqs-main branch from fce5e5a to c576065 Compare November 30, 2023 03:26

github-actions bot removed the T:build label Nov 30, 2023

p0mvn marked this pull request as ready for review November 30, 2023 03:30

Base automatically changed from roman/sqs-ingest-manager to main November 30, 2023 03:42

Merge branch 'main' into roman/sqs-main

4515605

czarcas7ic approved these changes Nov 30, 2023

View reviewed changes

p0mvn merged commit 4edf2cb into main Nov 30, 2023
1 check passed

p0mvn deleted the roman/sqs-main branch November 30, 2023 21:35

mergify bot mentioned this pull request Nov 30, 2023

feat: sidecar query server (backport #6953) #6962

Merged

mergify bot mentioned this pull request Nov 30, 2023

feat: sidecar query server (backport #6953) #6963

Merged

p0mvn added a commit that referenced this pull request Nov 30, 2023

feat: sidecar query server (backport #6953)

19762d9

p0mvn added a commit that referenced this pull request Nov 30, 2023

feat: sidecar query server (backport #6953) (#6962)

d810d99

Co-authored-by: roman <roman@osmosis.team>

mattverse reviewed Dec 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: sidecar query server #6953

feat: sidecar query server #6953

p0mvn commented Nov 30, 2023 •

edited

Loading

czarcas7ic left a comment

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

czarcas7ic Nov 30, 2023

p0mvn commented Nov 30, 2023 •

edited

Loading

mattverse Dec 1, 2023

mattverse Dec 1, 2023

mattverse Dec 1, 2023

mattverse Dec 1, 2023

mattverse Dec 1, 2023

mattverse Dec 1, 2023

mattverse Dec 1, 2023

mattverse Dec 1, 2023

		- How to handle atomicity between ticks and pools? E.g. let's say a block is written between the time initial pools are read
		and the time the ticks are read. Now, we have data that is partially up-to-date.

	// TODO: refactor get these directl from the pools repository.
	// TODO: refactor get these directly from the pools repository.

		@@ -0,0 +1,3 @@
		// Encapsulates the Model-View-Controller abstraction domain.

feat: sidecar query server #6953

feat: sidecar query server #6953

Conversation

p0mvn commented Nov 30, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

czarcas7ic 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

p0mvn commented Nov 30, 2023 • edited Loading

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

p0mvn commented Nov 30, 2023 •

edited

Loading

p0mvn commented Nov 30, 2023 •

edited

Loading