reduced origin min gas balance when quoting a route w/ origin gas token #3487
Conversation
parodime
requested review from
dwasse,
trajan0x and
aureliusbtc
as code owners
WalkthroughThe pull request introduces a new method Changes
Sequence DiagramsequenceDiagram
participant Quoter
participant InventoryManager
Quoter->>InventoryManager: HasSufficientGasWithMult(ctx, chainID, gasValue, multiplier)
InventoryManager-->>Quoter: Returns (sufficient, error)
alt Is Native Token
Quoter->>InventoryManager: Check with 0.65 multiplier
else Standard Token
Quoter->>InventoryManager: Check with default multiplier
end
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
8b05383
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eef95d50.sanguine-fe.pages.dev |
| Branch Preview URL: | https://parod-minquoteamt.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
services/rfq/relayer/inventory/manager.go (1)
Line range hint
446-474: Consider using constants for the multiplier range.The implementation is solid, but the magic numbers 0 and 2 for the multiplier range could be defined as constants for better maintainability and documentation.
+const ( + minThresholdMultiplier = 0.0 + maxThresholdMultiplier = 2.0 +) func (i *inventoryManagerImpl) HasSufficientGasWithMult(parentCtx context.Context, chainID int, gasValue *big.Int, thresholdMultiplier *float64) (sufficient bool, err error) { // ... - if *thresholdMultiplier < 0 || *thresholdMultiplier > 2 { + if *thresholdMultiplier < minThresholdMultiplier || *thresholdMultiplier > maxThresholdMultiplier { return false, fmt.Errorf("thresholdMultiplier out of range: %f", *thresholdMultiplier) } // ... }services/rfq/relayer/quoter/quoter.go (1)
775-784: Improve code maintainability with constants.Consider the following improvements:
- Define the 0.65 multiplier as a named constant
- Use
util.EthAddressinstead of the hardcoded address+const nativeGasTokenMultiplier = 0.65 + // if the origin token is native gas, require less minimum gas on the origin chain. // as origin gas gets critically low, this will discourage the relayer from quoting most routes *except* those that will replenish the deficit gas. var sufficentGasOrigin bool -if input.OriginTokenAddr.Hex() == "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" { - multiplier := 0.65 +if input.OriginTokenAddr == util.EthAddress { + multiplier := nativeGasTokenMultiplier sufficentGasOrigin, err = m.inventoryManager.HasSufficientGasWithMult(ctx, input.OriginChainID, nil, &multiplier) } else { sufficentGasOrigin, err = m.inventoryManager.HasSufficientGas(ctx, input.OriginChainID, nil) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/rfq/relayer/inventory/manager.go(3 hunks)services/rfq/relayer/inventory/mocks/manager.go(1 hunks)services/rfq/relayer/quoter/quoter.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
services/rfq/relayer/inventory/mocks/manager.go (1)
139-159: Clean mock implementation!The mock method follows the established mockery pattern and correctly handles all parameters and return values.
services/rfq/relayer/inventory/manager.go (2)
51-52: Well-documented interface addition!The new method is clearly documented and follows the interface's documentation style.
443-444: Good refactoring to reuse logic!The implementation maintains backward compatibility by delegating to the new method with a nil multiplier.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/relayer-arb-call #3487 +/- ##
=========================================================
Coverage 21.98636% 21.98636%
=========================================================
Files 502 502
Lines 41926 41926
=========================================================
Hits 9218 9218
Misses 31772 31772
Partials 936 936
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // if the origin token is native gas, require less minimum gas on the origin chain. | ||
| // as origin gas gets critically low, this will discourage the relayer from quoting most routes *except* those that will replenish the deficit gas. | ||
| var sufficentGasOrigin bool | ||
| if input.OriginTokenAddr.Hex() == "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" { |
There was a problem hiding this comment.
Can use util.IsGasToken() here instead of manual check
| // as origin gas gets critically low, this will discourage the relayer from quoting most routes *except* those that will replenish the deficit gas. | ||
| var sufficentGasOrigin bool | ||
| if input.OriginTokenAddr.Hex() == "0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" { | ||
| multiplier := 0.65 |
There was a problem hiding this comment.
I would say this should probably be configurable, and we can default to 0.65 or some other sensible value
| @@ -48,6 +48,8 @@ type Manager interface { | |||
| ApproveAllTokens(ctx context.Context) error | |||
| // HasSufficientGas checks if there is sufficient gas for a given route. | |||
| HasSufficientGas(ctx context.Context, chainID int, gasValue *big.Int) (bool, error) | |||
| // HasSufficientGasWithMult checks if there is sufficient gas for a given route with an optional threshold multiplier applied. | |||
| HasSufficientGasWithMult(ctx context.Context, chainID int, gasValue *big.Int, thresholdMultiplier *float64) (bool, error) | |||
There was a problem hiding this comment.
My suggestion here would be to add the thresholdMultiplier param to HasSufficientGas and remove HasSufficientGasWithMult- since you already have thresholdMultiplier as a pointer, it serves as optional param. This way we can keep the interface simple
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
as origin chain gets critically low on gas, this change should discourage the relayer from quoting most routes involving that chain except those that will replenish the deficit gas
Summary by CodeRabbit
New Features
Bug Fixes
Improvements