Dont attribute new revenue if user exceeds 30d max volume and deprecate AffiliateWhitelist #3109
Conversation
WalkthroughAdds affiliate overrides/parameters, threads per-fill affiliate fee quantums through rev-share and CLOB into stats and indexer protos, introduces windowed referred-volume persistence and expiration hooks, updates mocks/tests/interfaces, and bumps last affiliate tier PPM. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as CLOB Keeper
participant A as Affiliates Keeper
participant R as RevShare Keeper
participant S as Stats Keeper
Note over C: Match processing
C->>A: GetAffiliateOverridesMap()
C->>A: GetAffiliateParameters()
C->>R: GetAllRevShares(fill, overrides, params)
R->>A: GetTakerFeeShare(taker, overrides)
R-->>C: RevShares + affiliateFeeGenerated (quote quantums)
C->>S: RecordFill(taker, maker, notional, affiliateFeeGenerated)
S->>S: Persist BlockStats.Fill (incl. affiliate fee)
Note over S: EndBlock
S->>S: ProcessBlockStats → increment taker UserStats.AffiliateRevenueGeneratedQuantums
sequenceDiagram
autonumber
participant S as Stats Keeper
participant H as StatsExpirationHook (Affiliates)
participant A as Affiliates Keeper
Note over S: Stats expiration flow
S->>S: ExpireOldStats(user)
S->>H: OnStatsExpired(user, resultingUserStats)
H->>A: OnStatsExpired(user, resultingUserStats)
A->>A: Adjust windowed referred volume and persist via RVW:
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
protocol/x/affiliates/keeper/keeper_test.go (1)
725-935: Add missing commission assertions to test cases.The test defines
expectedAttributedComissionin 5 test cases but never asserts it. TheAggregateAffiliateReferredVolumeForFillsmethod (keeper.go:524) callsAddReferredCommissionto track affiliate fees, yet the test at lines 930-933 only validates volume. Add assertions usingGetReferredCommissionto verify commission tracking, particularly for the "reached maximum attributable revenue and commission" case that tests the cap at 2_500_000_000.referredCommission, err := k.GetReferredCommission(ctx, affiliate) require.NoError(t, err) require.Equal(t, tc.expectedAttributedComission, referredCommission)protocol/x/stats/keeper/keeper.go (1)
280-290: Critical: Missing affiliate revenue decrement in expiration logic.When expiring old stats, the function decrements
TakerNotionalandMakerNotionalfrom user stats (lines 283-284), but does NOT decrementAffiliateRevenueQuantums. This will cause affiliate revenue to accumulate indefinitely in user stats without ever rolling off after the window period.Apply this diff to decrement affiliate revenue when stats expire:
globalStats := k.GetGlobalStats(ctx) for _, removedStats := range epochStats.Stats { stats := k.GetUserStats(ctx, removedStats.User) stats.TakerNotional -= removedStats.Stats.TakerNotional stats.MakerNotional -= removedStats.Stats.MakerNotional + stats.AffiliateRevenueQuantums -= removedStats.Stats.AffiliateRevenueQuantums k.SetUserStats(ctx, removedStats.User, stats) // Just remove TakerNotional to avoid double counting globalStats.NotionalTraded -= removedStats.Stats.TakerNotional }
🧹 Nitpick comments (1)
protocol/x/stats/keeper/keeper_test.go (1)
28-33: Add test coverage for non-zero affiliate fees.The
recordFillArgsstruct correctly includes the newaffiliateFeefield, but all test cases usebig.NewInt(0). Consider adding test cases with non-zero affiliate fees to verify that affiliate revenue is correctly aggregated intoUserStats.AffiliateRevenueQuantums.Example test case to add:
+ "fill with affiliate fee": { + []recordFillArgs{ + {"alice", "bob", new(big.Int).SetUint64(100), big.NewInt(10)}, + }, + &types.BlockStats{ + Fills: []*types.BlockStats_Fill{ + { + Taker: "alice", + Maker: "bob", + Notional: 100, + AffiliateFeeGeneratedQuantums: 10, + }, + }, + }, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
protocol/x/stats/types/stats.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts(12 hunks)proto/dydxprotocol/stats/stats.proto(2 hunks)protocol/x/affiliates/keeper/keeper.go(7 hunks)protocol/x/affiliates/keeper/keeper_test.go(8 hunks)protocol/x/affiliates/types/keys.go(1 hunks)protocol/x/clob/keeper/process_single_match.go(1 hunks)protocol/x/clob/types/expected_keepers.go(1 hunks)protocol/x/stats/keeper/keeper.go(3 hunks)protocol/x/stats/keeper/keeper_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
PR: dydxprotocol/v4-chain#2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.
Applied to files:
protocol/x/stats/keeper/keeper_test.goprotocol/x/clob/types/expected_keepers.go
🧬 Code graph analysis (3)
protocol/x/affiliates/keeper/keeper_test.go (5)
protocol/testutil/app/app.go (1)
NewTestAppBuilder(335-350)protocol/x/affiliates/types/constants.go (1)
DefaultAffiliateTiers(4-32)protocol/x/affiliates/keeper/keeper.go (1)
Keeper(21-28)protocol/x/stats/types/stats.pb.go (6)
BlockStats(32-35)BlockStats(39-39)BlockStats(40-42)BlockStats_Fill(78-89)BlockStats_Fill(93-93)BlockStats_Fill(94-96)protocol/x/affiliates/types/affiliates.pb.go (3)
AffiliateParameters(241-249)AffiliateParameters(253-253)AffiliateParameters(254-256)
protocol/x/stats/keeper/keeper.go (2)
indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (2)
BlockStats_Fill(18-36)BlockStats_Fill(221-285)protocol/x/stats/types/stats.pb.go (3)
BlockStats_Fill(78-89)BlockStats_Fill(93-93)BlockStats_Fill(94-96)
protocol/x/affiliates/keeper/keeper.go (4)
protocol/x/affiliates/types/keys.go (1)
ReferredCommissionKeyPrefix(18-18)protocol/dtypes/serializable_int.go (2)
SerializableInt(13-15)NewIntFromBigInt(43-49)protocol/x/affiliates/types/errors.go (1)
ErrUpdatingAffiliateReferredVolume(9-10)protocol/lib/big_math.go (1)
BigU(10-12)
🪛 GitHub Actions: Protobuf
proto/dydxprotocol/stats/stats.proto
[error] 1-1: Formatting check failed: git diff --exit-code detected changes after clang-format. The process exited with code 1 indicating uncommitted formatting diffs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (38)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: test / run_command
- GitHub Check: benchmark
- GitHub Check: golangci-lint
- GitHub Check: build-and-push-mainnet
- GitHub Check: lint
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-coverage-upload
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: build
- GitHub Check: container-tests
- GitHub Check: build-and-push-testnet
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (17)
protocol/x/affiliates/types/keys.go (1)
18-18: LGTM!The new state key prefix follows the established pattern and aligns with the related
ReferredVolumeKeyPrefix.protocol/x/affiliates/keeper/keeper_test.go (2)
132-155: LGTM! Cap enforcement logic verified.The test correctly validates that
AddReferredVolumeenforces the maximum cap (1250) when the sum (1000 + 500 = 1500) would exceed it.
157-180: LGTM! Commission cap test correctly validates new API.The test properly verifies the
AddReferredCommissionfunction's cap enforcement: initial 50 + added 100 = 150, clamped to max 100.protocol/x/affiliates/keeper/keeper.go (2)
519-527: LGTM! Taker referred volume and commission tracking correctly implemented.The code properly applies 30-day caps to both volume and commission for taker fills, with clear comments explaining each step.
538-542: The asymmetry is intentional and correct by design.Affiliate fees are only generated from taker fees, not maker fees. The
getAffiliateRevSharesfunction inprotocol/x/revshare/keeper/revshare.go(lines 290-320) explicitly processes only the taker's address and taker fees, callingGetTakerFeeShareand returning revenue shares withREV_SHARE_FEE_SOURCE_TAKER_FEE. No corresponding logic exists for maker fees.The maker path correctly tracks only referred volume (line 539) for tier calculation purposes, while the taker path tracks both volume (line 520) and commission (line 524) because only takers generate affiliate fees. This design is confirmed by the function naming (
GetTakerFeeShare, notGetMakerFeeShare) and comments stating "taker_fee_share is only affiliate."Likely an incorrect or invalid review comment.
protocol/x/clob/keeper/process_single_match.go (1)
564-570: LGTM! Affiliate revenue parameter correctly propagated to stats.The change appropriately passes
affiliateRevSharesQuoteQuantumstoRecordFill, aligning with the updated interface. The value flows fromrevSharesForFill.AffiliateRevShare.QuoteQuantums(line 535) and is initialized tobig.NewInt(0)at line 334, ensuring it's never nil.protocol/x/stats/keeper/keeper_test.go (1)
89-89: RecordFill call correctly updated with new parameter.The function call properly includes the
affiliateFeeparameter, matching the updated signature.indexer/packages/v4-protos/src/codegen/dydxprotocol/stats/stats.ts (6)
212-237: Codegen correctly implements affiliate fee field for BlockStats_Fill.The generated code properly:
- Initializes
affiliateFeeGeneratedQuantumstoLong.UZERO- Encodes the field only when non-zero (protobuf optimization)
- Assigns field number 4 (tag >>> 3 = 4 → wire tag 32)
263-265: Decode logic correctly handles affiliate fee field.Case 4 properly reads the
affiliateFeeGeneratedQuantumsfield asuint64, consistent with the proto definition.
281-281: fromPartial correctly handles optional affiliate fee field.The null-coalescing to
Long.UZEROensures safe initialization when the field is absent.
487-507: Codegen correctly implements affiliate revenue field for UserStats.The generated code properly:
- Initializes
affiliateRevenueQuantumstoLong.UZERO- Encodes the field only when non-zero
- Assigns field number 3 (tag >>> 3 = 3 → wire tag 24)
529-531: Decode logic correctly handles affiliate revenue field.Case 3 properly reads the
affiliateRevenueQuantumsfield asuint64, consistent with the proto definition.
546-546: fromPartial correctly handles optional affiliate revenue field.The null-coalescing to
Long.UZEROensures safe initialization when the field is absent.protocol/x/clob/types/expected_keepers.go (1)
164-167: Interface signature correctly extended for affiliate tracking.The
RecordFillmethod signature now includes theaffiliateFeeGeneratedparameter, aligning with:
- The implementation call in
protocol/x/clob/keeper/process_single_match.go:569- The test updates in
protocol/x/stats/keeper/keeper_test.go:89- The proto field additions
Using
*big.Intis appropriate for quantum-denominated values.protocol/x/stats/keeper/keeper.go (3)
210-214: Affiliate revenue accumulation looks correct.The logic properly accumulates
AffiliateFeeGeneratedQuantumsinto the taker's persistent user stats. The comment at line 212 helpfully explains the purpose.Note: The overflow caveat mentioned in the comment at lines 207-208 applies to this field as well, though the required trading volume to overflow a uint64 is indeed unrealistic.
232-235: Epoch snapshot update is correct.The affiliate revenue is properly tracked in the epoch snapshot for the taker, consistent with how taker and maker notional values are tracked.
82-88: The nil pointer concern is unfounded.After thorough verification:
Production caller (
protocol/x/clob/keeper/process_single_match.go:569):
affiliateRevSharesQuoteQuantumsis initialized tobig.NewInt(0)(line 334)- Only reassigned from
revSharesForFill.AffiliateRevShare.QuoteQuantums(line 535)QuoteQuantumsis computed vialib.BigMulPpm()which always returns non-nil*big.IntTest caller (
protocol/x/stats/keeper/keeper_test.go:89):
- All test cases pass
big.NewInt(0)as theaffiliateFeeparameterThe
QuoteQuantumsfield, while declared as*big.Int, is consistently assigned non-nil values from mathematical operations that create newbig.Intinstances. The standard library'sbig.Intoperations never return nil.Likely an incorrect or invalid review comment.
proto/dydxprotocol/stats/stats.proto
Outdated
|
|
||
| // Affiliate revenue generated in quantums | ||
| uint64 affiliate_revenue_quantums = 3; |
There was a problem hiding this comment.
Fix proto formatting for affiliate_revenue_quantums field.
The field is correctly defined with field number 3 and includes clear documentation. However, formatting needs to be corrected per the pipeline failure.
The formatting will be fixed by running clang-format on the entire file as suggested above.
🤖 Prompt for AI Agents
In proto/dydxprotocol/stats/stats.proto around lines 68 to 70, the
affiliate_revenue_quantums field is fine semantically but fails the repository's
formatting checks; run clang-format (or the project's configured proto
formatter) on the entire file to normalize spacing/indentation and commit the
formatted file so the pipeline passes.
| testCases := []struct { | ||
| name string | ||
| referrals int | ||
| expectedVolume *big.Int | ||
| setup func(t *testing.T, ctx sdk.Context, k *keeper.Keeper, statsKeeper *statskeeper.Keeper) | ||
| name string | ||
| referrals int | ||
| expectedVolume *big.Int | ||
| expectedAttributedVolume *big.Int | ||
| expectedAttributedComission *big.Int | ||
| setup func(t *testing.T, ctx sdk.Context, k *keeper.Keeper, statsKeeper *statskeeper.Keeper) |
There was a problem hiding this comment.
Fix typo in field name: Comission → Commission.
The struct field is misspelled as expectedAttributedComission (missing an 'm').
Apply this diff:
- expectedAttributedComission *big.Int
+ expectedAttributedCommission *big.IntThen update all references in the test cases below (lines 735, 759, 780, 809, 848, 883).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testCases := []struct { | |
| name string | |
| referrals int | |
| expectedVolume *big.Int | |
| setup func(t *testing.T, ctx sdk.Context, k *keeper.Keeper, statsKeeper *statskeeper.Keeper) | |
| name string | |
| referrals int | |
| expectedVolume *big.Int | |
| expectedAttributedVolume *big.Int | |
| expectedAttributedComission *big.Int | |
| setup func(t *testing.T, ctx sdk.Context, k *keeper.Keeper, statsKeeper *statskeeper.Keeper) | |
| testCases := []struct { | |
| name string | |
| referrals int | |
| expectedVolume *big.Int | |
| expectedAttributedVolume *big.Int | |
| expectedAttributedCommission *big.Int | |
| setup func(t *testing.T, ctx sdk.Context, k *keeper.Keeper, statsKeeper *statskeeper.Keeper) |
🤖 Prompt for AI Agents
In protocol/x/affiliates/keeper/keeper_test.go around lines 730 to 736, rename
the struct field expectedAttributedComission to expectedAttributedCommission
(fix the typo) and update all references to that field in the test cases below
(lines ~735, 759, 780, 809, 848, 883) to use the new spelling; ensure any
assignments and assertions use expectedAttributedCommission and run tests to
confirm no other references remain.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
protocol/scripts/affiliates/update_affiliate_parameters.py (2)
120-124: Remove unnecessary f-string prefix.Line 122 uses an f-string without any placeholders. This issue was previously flagged and marked as resolved, but the fix was not applied.
Apply this diff:
- print(f"Loaded configuration from ~/.dydxprotocol/config/client.toml") + print("Loaded configuration from ~/.dydxprotocol/config/client.toml")
186-186: Critical: Hardcoded absolute path remains unfixed.Despite being flagged in previous reviews and marked as "Done" by the author, line 186 still contains the hardcoded user-specific path
/Users/justinbarnett/projects/v4-chain/protocol/build/dydxprotocold. This breaks portability and prevents other developers from using the script. Lines 36, 94, and 153 correctly use"dydxprotocold", but this line was missed.Apply this diff:
- "/Users/justinbarnett/projects/v4-chain/protocol/build/dydxprotocold", + "dydxprotocold",Alternatively, if you need to use a specific binary path, follow the environment variable approach suggested in the previous review to make it configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
protocol/scripts/affiliates/update_affiliate_parameters.py(6 hunks)protocol/x/affiliates/keeper/keeper.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
protocol/x/affiliates/keeper/keeper.go (6)
protocol/x/stats/keeper/keeper.go (1)
Keeper(23-31)protocol/x/affiliates/types/keys.go (1)
ReferredVolumeInWindowKeyPrefix(16-16)protocol/dtypes/serializable_int.go (1)
NewIntFromBigInt(43-49)protocol/x/affiliates/types/affiliates.pb.go (3)
AffiliateParameters(241-250)AffiliateParameters(254-254)AffiliateParameters(255-257)protocol/lib/big_math.go (1)
BigU(10-12)protocol/x/stats/types/stats.pb.go (3)
UserStats(356-363)UserStats(367-367)UserStats(368-370)
🪛 Ruff (0.13.3)
protocol/scripts/affiliates/update_affiliate_parameters.py
66-66: Do not catch blind exception: Exception
(BLE001)
122-122: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (38)
- GitHub Check: build
- GitHub Check: liveness-test
- GitHub Check: test-coverage-upload
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: container-tests
- GitHub Check: test / run_command
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: check-build-bazooka
- GitHub Check: benchmark
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: run_command
- GitHub Check: build-and-push-testnet
- GitHub Check: golangci-lint
- GitHub Check: build-and-push-mainnet
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (2)
protocol/scripts/affiliates/update_affiliate_parameters.py (2)
15-16: LGTM!The new imports support TOML configuration parsing and path handling for the client config feature.
137-146: LGTM!The parameter renaming from
max-30d-revenuetomax-30d-volumeis consistently applied across the argument name, field mapping, and summary text. The distinction between volume tracking (line 137) and revenue/commission limits (line 139) is correctly maintained.
| def load_client_config() -> Dict[str, Any]: | ||
| """ | ||
| Loads configuration from ~/.dydxprotocol/config/client.toml if it exists. | ||
| Returns: | ||
| Dictionary containing chain-id and node from client.toml, or empty dict if not found | ||
| """ | ||
| config_path = Path.home() / ".dydxprotocol" / "config" / "client.toml" | ||
| if config_path.exists(): | ||
| try: | ||
| with open(config_path, 'r') as f: | ||
| config = toml.load(f) | ||
| return config | ||
| except Exception as e: | ||
| print(f"Warning: Could not load client.toml: {e}") | ||
| return {} | ||
| return {} |
There was a problem hiding this comment.
Update docstring to match implementation.
The function documentation states it returns "chain-id and node from client.toml," but the implementation returns the entire TOML config dictionary. Update the docstring to accurately reflect the behavior.
Apply this diff:
def load_client_config() -> Dict[str, Any]:
"""
Loads configuration from ~/.dydxprotocol/config/client.toml if it exists.
Returns:
- Dictionary containing chain-id and node from client.toml, or empty dict if not found
+ Dictionary containing the entire client.toml configuration, or empty dict if not found
"""Additionally, consider validating that the returned config contains expected keys (e.g., 'chain-id', 'node') to catch configuration errors early.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def load_client_config() -> Dict[str, Any]: | |
| """ | |
| Loads configuration from ~/.dydxprotocol/config/client.toml if it exists. | |
| Returns: | |
| Dictionary containing chain-id and node from client.toml, or empty dict if not found | |
| """ | |
| config_path = Path.home() / ".dydxprotocol" / "config" / "client.toml" | |
| if config_path.exists(): | |
| try: | |
| with open(config_path, 'r') as f: | |
| config = toml.load(f) | |
| return config | |
| except Exception as e: | |
| print(f"Warning: Could not load client.toml: {e}") | |
| return {} | |
| return {} | |
| def load_client_config() -> Dict[str, Any]: | |
| """ | |
| Loads configuration from ~/.dydxprotocol/config/client.toml if it exists. | |
| Returns: | |
| Dictionary containing the entire client.toml configuration, or empty dict if not found | |
| """ | |
| config_path = Path.home() / ".dydxprotocol" / "config" / "client.toml" | |
| if config_path.exists(): | |
| try: | |
| with open(config_path, 'r') as f: | |
| config = toml.load(f) | |
| return config | |
| except Exception as e: | |
| print(f"Warning: Could not load client.toml: {e}") | |
| return {} | |
| return {} |
🧰 Tools
🪛 Ruff (0.13.3)
66-66: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In protocol/scripts/affiliates/update_affiliate_parameters.py around lines 53 to
69, the docstring inaccurately states the function returns "chain-id and node
from client.toml" while the implementation returns the entire TOML config dict;
update the docstring to state it returns the full configuration dictionary (or
an empty dict if file missing/parse fails). Also add simple validation after
loading: check for presence of expected keys like 'chain-id' and 'node' and log
or raise a clear error (or return empty dict) if they are missing to surface
misconfiguration early.
| # Print the full command | ||
| result = subprocess.run(cmd, capture_output=True, text=True) |
There was a problem hiding this comment.
Implement the command printing or remove the comment.
The comment indicates the full command should be printed, but there's no corresponding print statement.
Apply this diff to print the command before execution:
# Print the full command
+print(f"Executing command: {' '.join(cmd)}")
result = subprocess.run(cmd, capture_output=True, text=True)Or remove the comment if printing is not needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Print the full command | |
| result = subprocess.run(cmd, capture_output=True, text=True) | |
| # Print the full command | |
| print(f"Executing command: {' '.join(cmd)}") | |
| result = subprocess.run(cmd, capture_output=True, text=True) |
🧰 Tools
🪛 Ruff (0.13.3)
168-168: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
In protocol/scripts/affiliates/update_affiliate_parameters.py around lines
167-168, the comment says "Print the full command" but the code does not print
it; either remove the comment or add a print/logging statement before
subprocess.run to output the full command (join the cmd list into a single
string for readability or use the logger to emit the command), then call
subprocess.run as-is so the command is visible in logs.
|
https://github.com/Mergifyio backport release/protocol/v9.x |
✅ Backports have been created
|
…te AffiliateWhitelist (#3109) (cherry picked from commit c29eea2) # Conflicts: # protocol/app/testdata/default_genesis_state.json # protocol/scripts/affiliates/update_affiliate_parameters.py # protocol/scripts/genesis/sample_pregenesis.json # protocol/x/affiliates/keeper/keeper.go # protocol/x/affiliates/keeper/keeper_test.go # protocol/x/affiliates/types/constants.go # protocol/x/revshare/keeper/revshare_test.go
|
https://github.com/Mergifyio backport release/protocol/v9.x |
✅ Backports have been created
|
…te AffiliateWhitelist (#3109)
|
https://github.com/Mergifyio backport release/indexer/v9.x |
✅ Backports have been created
|
…te AffiliateWhitelist (#3109)
Changelist
Test Plan
Side Effects:
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Changes
Documentation
Chores