Skip to content

feat: implement logic to end vaults season one#48

Merged
johnletey merged 24 commits into
mainfrom
luca/vault-program-end
Jul 30, 2025
Merged

feat: implement logic to end vaults season one#48
johnletey merged 24 commits into
mainfrom
luca/vault-program-end

Conversation

@g-luca
Copy link
Copy Markdown
Member

@g-luca g-luca commented Jul 11, 2025

Summary by CodeRabbit

  • New Features

    • Introduced automatic termination of Vaults Season One after a specified timestamp, pausing flexible vault lock/unlock actions post-season.
    • Added an interim period where Staked Vault yield is redirected to a configured collector address for Season Two.
    • New configuration parameters for Vaults Season One end timestamp and Season Two yield collector address.
    • Enhanced state tracking and export for Vaults Season One completion.
    • Added block lifecycle processing to trigger season end logic.
  • Bug Fixes

    • Improved validation and error handling for vault operations after season end.
  • Tests

    • Added comprehensive tests covering Vaults Season One end and Season Two behaviors.
  • Documentation

    • Updated changelog and documentation to reflect Vaults lifecycle enhancements.

@g-luca g-luca requested a review from johnletey July 11, 2025 10:43
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jul 11, 2025

Walkthrough

This update introduces the concept of "Vaults Season One" with a configurable end timestamp and a collector for Staked Vault yield during Season Two. It adds new fields to protobuf schemas, Go structs, and configuration files, implements time-based logic for vault operations including a BeginBlocker to end Season One, refactors vault yield handling, extends genesis state handling, and adds comprehensive tests validating the new behaviors.

Changes

Cohort / File(s) Change Summary
Protobuf: Module and Genesis Updates
proto/noble/dollar/module/v1/module.proto, proto/noble/dollar/vaults/v1/genesis.proto, api/module/v1/module.pulsar.go, api/vaults/v1/genesis.pulsar.go
Add fields for vaults season one end timestamp, season two yield collector, and season one ended state to module and genesis messages; update Go structs and reflection logic accordingly.
Keeper Struct, State, and Logic
keeper/keeper.go, keeper/state_vaults.go, types/vaults/keys.go
Extend Keeper with new fields for season one end timestamp, season two yield collector, and season one ended flag; add getter, state key constant, and method to check if season one ended.
Vaults Msg Server & Logic Refactor
keeper/msg_server.go, keeper/msg_server_vaults.go
Refactor yield handling to separate season one and season two logic; add internal unlock helper and season one end logic; enforce time-based lock/unlock restrictions for Flexible vaults after season one end.
Block Lifecycle & Module Wiring
keeper/abci.go, module.go
Add BeginBlocker logic to end season one after configured timestamp with panic recovery; wire up BeginBlocker and new keeper parameters in module initialization and configuration validation.
Genesis Handling
genesis.go
Extend genesis import/export to include season one ended state in vaults module.
Configuration & Mocks
simapp/app.yaml, utils/mocks/dollar.go
Add new config parameters for season one end timestamp and season two yield collector address; update mocks to supply new keeper arguments.
Changelog Documentation
.changelog/v2.1.0/features/48-season-one.md, .changelog/v2.1.0/summary.md, CHANGELOG.md
Add changelog entries describing the new season one end feature and release summary.
Tests
keeper/msg_server_vaults_test.go
Add tests for vaults season one end and season two yield collector behaviors, covering user flows, balances, yield claiming, and lock/unlock restrictions.
Build & Protobuf Tooling
Makefile
Update protobuf breaking check to compare against a newer branch version.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant MsgServer
    participant Keeper
    participant Block
    participant State

    Note over Block: At BeginBlock
    Block->>Keeper: BeginBlocker(ctx)
    Keeper->>State: Check season one ended flag & timestamp
    alt If season ended and not marked
        Keeper->>Keeper: endVaultsSeasonOne(ctx)
        Keeper->>State: Set season one ended flag
    end

    User->>MsgServer: Lock/Unlock Flexible Vault
    MsgServer->>Keeper: Check block time vs season one end
    alt After season one end
        MsgServer-->>User: Error: action paused
    else Before season one end
        MsgServer->>Keeper: Proceed with lock/unlock
    end

    User->>MsgServer: UpdateIndex
    MsgServer->>Keeper: IsVaultsSeasonOneEnded(ctx)
    alt If not ended
        Keeper->>Keeper: handleVaultsYieldSeasonOne
    else If ended
        Keeper->>Keeper: handleVaultsYieldSeasonTwo
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • johnletey

Poem

The season has ended, the vaults take a rest,
With timestamps and collectors, we code at our best.
Block time ticks onward, the logic is clear,
Flexible vaults pause, Staked yields reappear.
Genesis remembers, the changelogs delight—
A bunny hops forward, reviewing all night! 🐇✨

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0349418 and b1a8876.

📒 Files selected for processing (2)
  • keeper/abci.go (1 hunks)
  • keeper/msg_server.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • keeper/abci.go
  • keeper/msg_server.go
⏰ 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). (1)
  • GitHub Check: build
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch luca/vault-program-end

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@g-luca g-luca requested review from 0xstepit and jtieri July 11, 2025 10:44
Copy link
Copy Markdown
Member

@johnletey johnletey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great so far @g-luca! Have a few small nits, so feel free to ignore any 😄

Regarding the logs, we might need to adjust those. They are in POSIX format, but they don't quite match other logs in the rest of Noble.

Comment thread keeper/abci.go Outdated
Comment thread keeper/state_vaults.go Outdated
Comment thread proto/noble/dollar/module/v1/module.proto Outdated
Comment thread simapp/app.yaml Outdated
Comment thread keeper/abci.go Outdated
Comment thread keeper/abci.go Outdated
Comment thread keeper/abci.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
@johnletey johnletey marked this pull request as ready for review July 15, 2025 06:01
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (8)
keeper/state_vaults.go (1)

86-86: Fix grammatical error in comment.

The comment has a grammatical error: "that return" should be "that returns".

simapp/app.yaml (1)

75-75: Address timestamp concern for CI/E2E tests.

The timestamp 1752223200 (≈ May 11, 2025) could cause E2E tests to fail if CI runs after this date, as the vaults program would be considered ended. Consider using a much further future timestamp or making this configurable per environment to prevent unexpected test failures.

#!/bin/bash
# Description: Check the current date and verify how close we are to the configured timestamp

# Current timestamp
current_timestamp=$(date +%s)
echo "Current timestamp: $current_timestamp ($(date))"

# Configured timestamp
config_timestamp=1752223200
echo "Configured timestamp: $config_timestamp ($(date -d @$config_timestamp))"

# Calculate difference
diff=$((config_timestamp - current_timestamp))
days=$((diff / 86400))

if [ $diff -gt 0 ]; then
    echo "Time remaining: $days days"
else
    echo "WARNING: Timestamp has already passed! Tests may fail."
fi
keeper/abci.go (4)

26-29: Fix import ordering per goimports-reviser standard.

-	"dollar.noble.xyz/v2/types/vaults"
-
-	"github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types"
+
+	"dollar.noble.xyz/v2/types/vaults"

35-35: Improve the error message for clarity.

-			k.logger.Error("recovered from x/dollar BeginBlocker panic handling the program end", r)
+			k.logger.Error("recovered from x/dollar BeginBlocker panic handling the vaults program end", "err", r)

47-49: Add a blank line before the comment.

 		}()
+
 		// Create a cached Context for the execution.

52-52: Improve the error message for clarity.

-			k.logger.Error("error executing the Vaults end program logic", err)
+			k.logger.Error("failed to execute vaults end program logic", "err", err)
keeper/msg_server_vaults.go (2)

441-441: Apply the suggested comment improvement.


443-443: Apply the suggested comment improvement.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc9cd20 and 83578c8.

📒 Files selected for processing (12)
  • api/module/v1/module.pulsar.go (16 hunks)
  • keeper/abci.go (1 hunks)
  • keeper/keeper.go (5 hunks)
  • keeper/msg_server.go (2 hunks)
  • keeper/msg_server_vaults.go (2 hunks)
  • keeper/msg_server_vaults_test.go (1 hunks)
  • keeper/state_vaults.go (1 hunks)
  • module.go (2 hunks)
  • proto/noble/dollar/module/v1/module.proto (1 hunks)
  • simapp/app.yaml (2 hunks)
  • types/vaults/keys.go (1 hunks)
  • utils/mocks/dollar.go (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
keeper/state_vaults.go (1)
keeper/keeper.go (1)
  • Keeper (50-91)
keeper/keeper.go (1)
types/vaults/keys.go (1)
  • ProgramEndedKey (44-44)
🔇 Additional comments (14)
keeper/state_vaults.go (1)

87-89: Verify error handling approach is intentional.

The method ignores all errors from VaultsProgramEnded.Get(ctx). While this pattern returns false by default when the state hasn't been set (which seems correct for a boolean flag), verify that ignoring other potential errors (like storage access issues) is the intended behavior.

proto/noble/dollar/module/v1/module.proto (1)

24-25: LGTM! Field addition is well-documented.

The new timestamp field is properly typed and documented, addressing the previous review feedback.

types/vaults/keys.go (1)

44-44: LGTM! Key constant follows established patterns.

The new storage key is properly named and follows the existing conventions for vault-related state keys.

utils/mocks/dollar.go (1)

75-75: Verify timestamp consistency with configuration.

The mock uses timestamp 1893499200 (≈ December 2029) while simapp/app.yaml uses 1752223200 (≈ May 2025). Confirm this difference is intentional - using a far future timestamp in mocks is likely safer for testing, but verify this aligns with the testing strategy.

simapp/app.yaml (1)

6-6: LGTM! Enables vaults program end logic.

Adding "dollar" to the begin_blockers list correctly enables the new BeginBlock lifecycle method for vaults program management.

module.go (2)

172-174: LGTM!

The BeginBlock method correctly delegates to the keeper's BeginBlocker and properly propagates any errors.


418-418: LGTM!

The VaultsEndProgramTimestamp configuration parameter is correctly passed to the keeper constructor.

keeper/keeper.go (1)

87-87: LGTM!

The VaultsProgramEnded collection item is correctly initialized with the appropriate key and type.

Also applies to: 162-162

keeper/msg_server.go (2)

163-168: LGTM!

The conditional check correctly prevents vault yield operations after the program has ended.


425-459: Well-structured refactoring!

The handleVaultsYield method cleanly extracts the vault yield logic while preserving the original functionality and error handling.

api/module/v1/module.pulsar.go (1)

17-22: LGTM!

The generated protobuf code correctly implements the new VaultsEndProgramTimestamp field with proper reflection, serialization, and accessor methods.

Also applies to: 32-32, 124-129, 153-154, 179-180, 209-211, 240-241, 270-271, 293-294, 378-380, 410-414, 590-608, 672-673, 724-729, 750-754

keeper/msg_server_vaults.go (2)

152-154: Good refactoring to enable code reuse.

The delegation pattern allows the internal unlock method to be called both from the public message server interface and internally from VaultsEndProgram.


493-506: Fix counter increment logic and log message format.

Same issue as above - counter incremented on failure and log format reversed.

-		}); err != nil {
-			k.logger.Error(fmt.Sprintf("Failed to unlock flexible vault position for %s: %v", flexibleUserAddr, err))
-			continue
-		}
-		flexibleUsersProcessed += 1
-	}
-	k.logger.Info(fmt.Sprintf("Unlocked %d/%d flexible vault positions!", len(flexibleUsers), flexibleUsersProcessed))
+		}); err != nil {
+			k.logger.Error(fmt.Sprintf("Failed to unlock flexible vault position for %s: %v", flexibleUserAddr, err))
+			continue
+		}
+		flexibleUsersProcessed++
+	}
+	k.logger.Info(fmt.Sprintf("Unlocked %d/%d flexible vault positions!", flexibleUsersProcessed, len(flexibleUsers)))

Likely an incorrect or invalid review comment.

keeper/msg_server_vaults_test.go (1)

1894-2032: Well-structured and comprehensive test coverage!

The test thoroughly validates the vaults program end functionality including:

  • Proper unlocking of all positions with correct yield/rewards distribution
  • Clearing of position state
  • Pausing of vault operations
  • Continued functionality of index updates

Comment thread keeper/keeper.go Outdated
Comment thread keeper/abci.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Copy link
Copy Markdown
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably will not need it, but in case we should ever call InitGenesis after the vaults program has ended and the BeginBlocker is still in place, then the logic would be tried to execute again.

We could add the corresponding VaultsProgramEnded to the genesis state akin to the Vaults setting etc.:

dollar/genesis.go

Lines 160 to 170 in 3cbdeeb

if err = k.VaultsPaused.Set(ctx, int32(genesis.Vaults.Paused)); err != nil {
panic(errors.Wrap(err, "unable to set genesis vaults paused state"))
}
if err = k.VaultsStats.Set(ctx, genesis.Vaults.Stats); err != nil {
panic(errors.Wrapf(err, "unable to set genesis vaults stats"))
}
if err = k.VaultsTotalFlexiblePrincipal.Set(ctx, genesis.Vaults.TotalFlexiblePrincipal); err != nil {
panic(errors.Wrap(err, "unable to set total vaults flexible principal"))
}

As mentioned probably not necessary but could be cleaner 👀

Copy link
Copy Markdown
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work @g-luca! nicely done; I left some comments (mostly nits) -- feel free to close them if you don't agree 🙏

Comment thread keeper/msg_server.go Outdated
Comment thread keeper/msg_server.go
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults_test.go
Copy link
Copy Markdown
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job here @g-luca! Added a couple of nits and a fix suggestion for how we handle stakedUsers and flexibleUsers.

Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go
@g-luca g-luca requested a review from 0xstepit July 21, 2025 12:41
Copy link
Copy Markdown
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@johnletey johnletey self-requested a review July 22, 2025 11:43
Copy link
Copy Markdown
Member

@johnletey johnletey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there @g-luca, just a few more nits 😄

Comment thread genesis.go Outdated
Comment thread keeper/abci.go Outdated
Comment thread keeper/abci.go Outdated
Comment thread keeper/abci.go Outdated
Comment thread keeper/abci.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
@johnletey johnletey changed the title feat: implement logic to handle end of Vaults program feat: implement logic to end vaults program Jul 22, 2025
@g-luca g-luca force-pushed the luca/vault-program-end branch from a38e571 to 61be38e Compare July 22, 2025 13:54
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (2)
proto/noble/dollar/vaults/v1/tx.proto (1)

64-74: Field number change creates protobuf breaking change.

Adding the vault field enables per-vault pause control, which is excellent for granular management. However, moving the paused field from number 2 to 3 breaks protobuf wire format compatibility.

To maintain compatibility, consider:

 message MsgSetPausedState {
   // ... existing fields ...
   string signer = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
-  vaults.v1.VaultType vault = 2;
-  vaults.v1.PausedType paused = 3;
+  vaults.v1.PausedType paused = 2;  // Keep original field number
+  vaults.v1.VaultType vault = 3;    // Use next available field number
 }

If this breaking change is intentional for v2.0.0, ensure it's properly documented.

api/vaults/v1/events.pulsar.go (1)

1-1: Revert manual changes to generated code & update the .proto instead

You’ve directly edited api/vaults/v1/events.pulsar.go, but that file is auto-generated. Those changes will be lost on the next regeneration. Please:

  • Revert any edits in api/vaults/v1/events.pulsar.go (and any other // Code generated … DO NOT EDIT files).
  • Open proto/noble/dollar/vaults/v1/events.proto and add the new Vault field (with the correct field number).
  • Run your protobuf generator (e.g. protoc --go-pulsar_out=…) to produce updated Go code.
♻️ Duplicate comments (1)
keeper/msg_server_vaults_test.go (1)

1970-1971: Consider using meaningful variable names for dates.

🧹 Nitpick comments (3)
Makefile (1)

41-41: Consider the impact of removing proto-breaking from standard workflow.

Removing proto-breaking from the proto-all target means breaking change checks won't run automatically. While this may be intentional for v2.0.0 development, consider whether this removes an important safety net for ongoing development.

keeper/state_vaults.go (2)

92-99: Consider improving error handling to distinguish between different error types.

The current implementation treats all errors the same way by returning vaults.NONE. While this is appropriate for "key not found" scenarios, other types of errors (like database connection issues) might warrant different handling or at least logging.

Consider this approach to distinguish between different error scenarios:

func (k *Keeper) IsVaultPaused(ctx context.Context, vault vaults.VaultType) vaults.PausedType {
	value, err := k.VaultsPaused.Get(ctx, int32(vault))
-	if err != nil {
+	if err != nil {
+		// Log non-NotFound errors for debugging
+		if !collections.IsNotFound(err) {
+			// Consider logging the error here
+		}
		return vaults.NONE
	}
	return vaults.PausedType(value)
}

101-105: LGTM! Simple and effective implementation.

The method correctly implements the program end status check with appropriate default behavior (returning false when the state is not found). The implementation aligns with the PR objective of adding logic to end the vaults program.

Optional improvement: Similar to IsVaultPaused, consider logging non-NotFound errors for debugging purposes while maintaining the current return behavior:

func (k *Keeper) IsVaultsProgramEnded(ctx context.Context) bool {
-	ended, _ := k.VaultsProgramEnded.Get(ctx)
+	ended, err := k.VaultsProgramEnded.Get(ctx)
+	if err != nil && !collections.IsNotFound(err) {
+		// Consider logging the error here for debugging
+	}
	return ended
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61be38e and af372e8.

⛔ Files ignored due to path filters (4)
  • types/vaults/events.pb.go is excluded by !**/*.pb.go
  • types/vaults/genesis.pb.go is excluded by !**/*.pb.go
  • types/vaults/query.pb.go is excluded by !**/*.pb.go
  • types/vaults/tx.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (17)
  • Makefile (1 hunks)
  • api/vaults/v1/events.pulsar.go (14 hunks)
  • api/vaults/v1/genesis.pulsar.go (24 hunks)
  • api/vaults/v1/query.pulsar.go (19 hunks)
  • api/vaults/v1/tx.pulsar.go (15 hunks)
  • genesis.go (3 hunks)
  • keeper/abci.go (1 hunks)
  • keeper/keeper.go (5 hunks)
  • keeper/msg_server_vaults.go (5 hunks)
  • keeper/msg_server_vaults_test.go (4 hunks)
  • keeper/query_server_vaults.go (1 hunks)
  • keeper/state_vaults.go (1 hunks)
  • proto/noble/dollar/vaults/v1/events.proto (1 hunks)
  • proto/noble/dollar/vaults/v1/genesis.proto (1 hunks)
  • proto/noble/dollar/vaults/v1/query.proto (1 hunks)
  • proto/noble/dollar/vaults/v1/tx.proto (1 hunks)
  • types/vaults/keys.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • genesis.go
  • proto/noble/dollar/vaults/v1/genesis.proto
  • types/vaults/keys.go
  • keeper/abci.go
  • keeper/keeper.go
  • keeper/msg_server_vaults.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/vaults/v1/events.pulsar.go (1)
types/vaults/events.pb.go (3)
  • PausedStateUpdated (157-160)
  • PausedStateUpdated (164-164)
  • PausedStateUpdated (165-167)
⏰ 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). (2)
  • GitHub Check: prepare
  • GitHub Check: build
🔇 Additional comments (18)
proto/noble/dollar/vaults/v1/query.proto (1)

54-56: Approve the architectural improvement to per-vault paused states.

The change from a single paused field to a map<string, string> enables granular control over pause states for individual vault types, which is a significant improvement. This breaking change aligns with the broader refactor to support per-vault pause management and program lifecycle controls.

Makefile (1)

46-46: LGTM: Version update aligns with v2.0.0 release.

The branch reference update from v1.0.2 to v2.0.0 correctly aligns with the major version upgrade and breaking changes introduced in this PR.

keeper/query_server_vaults.go (1)

44-61: LGTM: Implementation correctly handles per-vault paused states.

The refactored Paused method properly:

  • Iterates over the VaultsPaused collection using Walk
  • Converts enum values to strings for the response
  • Maintains proper error handling
  • Returns a map structure consistent with the updated protobuf definition

The implementation aligns well with the architectural shift to per-vault pause management.

keeper/state_vaults.go (1)

77-90: LGTM! Clean implementation of the paused state map retrieval.

The method correctly iterates over all vault paused states using Walk and populates a map with proper error handling. The implementation aligns well with the architectural change from single paused state to per-vault paused states.

api/vaults/v1/tx.pulsar.go (6)

1798-1798: LGTM: Field descriptor additions are correct.

The addition of fd_MsgSetPausedState_vault field descriptor and its initialization in the init() function follows the standard protobuf code generation pattern and is consistent with other field descriptors in the file.

Also applies to: 1806-1806


1881-1886: LGTM: Range method correctly handles the new vault field.

The vault field handling in the Range method properly:

  • Checks for non-zero enum value (appropriate for proto3)
  • Uses correct enum conversion with protoreflect.ValueOfEnum
  • Maintains consistent early return pattern

1910-1911: LGTM: All reflection methods correctly handle the vault field.

The vault field is properly implemented across all reflection methods:

  • Has: Correctly checks x.Vault != 0 for proto3 enum semantics
  • Clear: Properly resets to zero value
  • Get/Set: Correct enum type conversions using protoreflect.ValueOfEnum and VaultType casting
  • Mutable: Appropriately panics for scalar enum field
  • NewField: Returns correct default enum value

Also applies to: 1932-1933, 1955-1957, 1983-1984, 2009-2010, 2028-2029


2105-2107: LGTM: Serialization logic correctly handles field reordering.

The marshaling updates properly:

  • Add size calculation for the vault field using varint encoding
  • Assign correct wire tags: vault gets 0x10 (field 2), paused moves to 0x18 (field 3)
  • Maintain efficient reverse-order encoding pattern

Also applies to: 2143-2149


2238-2257: LGTM: Unmarshaling correctly handles the new vault field.

The vault field unmarshaling implementation:

  • Properly validates wire type for varint encoding
  • Uses standard varint decoding loop with bit shifting
  • Correctly casts decoded value to VaultType
  • Maintains consistent error handling patterns

The field number update (paused moved from case 2 to case 3) is also correct.


2845-2846: LGTM: Message structure and getter method correctly implement field reordering.

The MsgSetPausedState updates properly:

  • Add Vault field with correct protobuf field number 2
  • Move Paused field to field number 3
  • Provide standard GetVault() getter with appropriate nil checking and default return value

This correctly implements the per-vault paused state functionality described in the PR objectives.

Also applies to: 2876-2881

keeper/msg_server_vaults_test.go (3)

58-58: LGTM! Consistent with per-vault paused state refactoring.

The change from GetVaultsPaused to IsVaultPaused with a specific vault type aligns with the broader refactoring to support per-vault paused states.


66-67: Good simplification of vault type usage.

The consistent use of vaults.STAKED instead of the verbose vaults.VaultType(vaultsv1.VaultType_STAKED) improves code readability throughout the test.

Also applies to: 74-75, 82-83, 86-86, 93-94, 101-102, 111-112, 115-115, 120-121, 128-129, 138-139, 142-142, 147-148, 155-156


1898-2034: Comprehensive test coverage for vaults program end feature.

The test thoroughly validates:

  • Automatic unlocking of all positions when the program ends
  • Correct distribution of yields and rewards to users
  • Proper pausing of flexible vault operations while staked vault remains operational
  • Index updates continue to work without errors after program end

The test implementation aligns well with the feature requirements.

api/vaults/v1/genesis.pulsar.go (4)

15-15: LGTM! Map implementation for per-vault paused states.

The new _GenesisState_5_map wrapper correctly implements the map interface for tracking paused states per vault type, supporting the transition from a global paused state to per-vault paused states.

Also applies to: 121-201


203-211: Field descriptors updated correctly for new structure.

The addition of fd_GenesisState_program_ended and proper initialization of field descriptors aligns with the new genesis state structure.

Also applies to: 219-222


307-324: Protobuf reflection methods properly updated.

All reflection methods (Range, Has, Clear, Get, Set, Mutable, NewField) have been correctly updated to handle:

  • The paused field as a map instead of an enum
  • The new program_ended boolean field

The implementation maintains consistency with protobuf conventions.

Also applies to: 346-351, 374-379, 411-422, 453-460, 493-498, 529-536


1148-1152: Genesis state structure aligns with vaults program lifecycle enhancements.

The structural changes to GenesisState:

  • Paused as map[int32]int32 enables per-vault-type paused state tracking
  • ProgramEnded boolean field tracks whether the vaults program has terminated

These changes properly support the new vaults program end feature.

Also applies to: 1196-1201, 1210-1215

api/vaults/v1/query.pulsar.go (1)

1-5445: Auto-generated code changes look correct

This is auto-generated protobuf code that correctly implements the transition from a single paused state to per-vault paused states. The changes include:

  • Proper implementation of the map wrapper type for the new map[string]string Paused field
  • Deterministic key sorting during marshaling to ensure consistent serialization
  • Correct handling of the map structure in all reflection, marshal, and unmarshal methods

Comment thread proto/noble/dollar/vaults/v1/events.proto
@g-luca g-luca force-pushed the luca/vault-program-end branch from f7f4739 to 61be38e Compare July 29, 2025 13:16
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
proto/noble/dollar/module/v1/module.proto (1)

24-25: Past review suggestion not fully addressed.

The past review comment suggested improving the comment to "vaults_end_program_timestamp defines the timestamp in seconds when the vaults program ends." However, the current comment uses "Vaults Season One ends" which is more specific but inconsistent with the general "vaults program" terminology used elsewhere.

Consider using the suggested wording from the past review for consistency, or ensure the specific "Season One" terminology is used consistently throughout the codebase.

keeper/msg_server.go (1)

430-466: Review the collections usage pattern.

The logic for getting total flexible principal uses both .Has() and .Get() which appears redundant since .Get() would return zero if not set. However, based on past review comments, this pattern is intentionally kept for testing coverage purposes.

The collections usage pattern (lines 447-453) was previously flagged as potentially redundant but kept for testing coverage. This is acceptable if it serves the testing strategy.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4d9d77d and 6c321da.

⛔ Files ignored due to path filters (1)
  • types/vaults/genesis.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (12)
  • Makefile (1 hunks)
  • api/module/v1/module.pulsar.go (16 hunks)
  • api/vaults/v1/genesis.pulsar.go (16 hunks)
  • keeper/abci.go (1 hunks)
  • keeper/keeper.go (6 hunks)
  • keeper/msg_server.go (2 hunks)
  • keeper/msg_server_vaults.go (4 hunks)
  • keeper/msg_server_vaults_test.go (2 hunks)
  • keeper/state_vaults.go (1 hunks)
  • module.go (3 hunks)
  • proto/noble/dollar/module/v1/module.proto (1 hunks)
  • proto/noble/dollar/vaults/v1/genesis.proto (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • Makefile
🚧 Files skipped from review as they are similar to previous changes (9)
  • keeper/keeper.go
  • keeper/msg_server_vaults.go
  • proto/noble/dollar/vaults/v1/genesis.proto
  • keeper/state_vaults.go
  • module.go
  • keeper/msg_server_vaults_test.go
  • api/vaults/v1/genesis.pulsar.go
  • keeper/abci.go
  • api/module/v1/module.pulsar.go
⏰ 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). (1)
  • GitHub Check: build
🔇 Additional comments (4)
proto/noble/dollar/module/v1/module.proto (1)

27-28: LGTM! Clear and descriptive field definition.

The field name and comment clearly describe the purpose of collecting Staked vault yield during the interim period.

keeper/msg_server.go (3)

163-173: Well-structured conditional yield handling logic.

The conditional logic properly separates yield handling between Season One and the interim period. The comment clearly explains the behavior, and error handling is consistent.


430-466: Verify error handling in reward record creation.

The method properly claims yields from both vault types and creates a reward record. The error handling is consistent throughout the method.


484-488: Potential issue with coin transfer logic.

The method transfers coins directly from the Staked vault address to the collector address. However, the yield was already claimed to the Staked vault address in the previous step, so this transfer should work correctly.

Comment thread keeper/msg_server.go Outdated
@0xstepit 0xstepit self-requested a review July 29, 2025 15:37
Copy link
Copy Markdown
Contributor

@boojamya boojamya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Luca! Minor comments.
I'm going to wait to approve until after the walk through tomorrow.

Comment thread proto/noble/dollar/module/v1/module.proto Outdated
Comment thread keeper/msg_server_vaults_test.go Outdated
Comment thread keeper/msg_server_vaults_test.go Outdated
@johnletey johnletey changed the title feat: implement logic to end vaults program feat: implement logic to end vaults season one Jul 30, 2025
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
.changelog/v2.1.0/features/48-season-one.md (1)

1-1: Refine the changelog bullet for clarity & style

The project’s changelog generally uses imperative mood (“Add”, “Remove”, “Fix”) and captures the key parameters introduced. A small wording tweak makes the entry clearer while staying concise.

- - Implement `BeginBlocker` to end Vaults Season One after a specific timestamp. ([#48](https://github.com/noble-assets/dollar/pull/48))
+ - Add `BeginBlocker` that terminates Vaults Season One when block time exceeds the configured `VaultsSeasonOneEndTimestamp`. ([#48](https://github.com/noble-assets/dollar/pull/48))
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 492a723 and 34faa03.

📒 Files selected for processing (4)
  • .changelog/v2.1.0/features/48-season-one.md (1 hunks)
  • .changelog/v2.1.0/summary.md (1 hunks)
  • CHANGELOG.md (1 hunks)
  • Makefile (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • CHANGELOG.md
  • .changelog/v2.1.0/summary.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • Makefile

Copy link
Copy Markdown
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here @g-luca! Before approving would like to have the log in the defer of the BegiBlocker, but everything else looks great!

Comment thread keeper/abci.go Outdated
Comment thread keeper/keeper.go
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go Outdated
Comment thread keeper/msg_server_vaults.go
Comment thread keeper/msg_server_vaults_test.go Outdated
Comment thread keeper/msg_server_vaults_test.go Outdated
Comment thread keeper/msg_server_vaults_test.go Outdated
Comment thread keeper/msg_server_vaults_test.go Outdated
Comment thread keeper/msg_server_vaults_test.go Outdated
Copy link
Copy Markdown
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! left a few comments but no vulns noticed :)

Comment thread CHANGELOG.md
Comment thread keeper/keeper.go
Comment thread keeper/msg_server.go
Comment thread keeper/msg_server_vaults.go
Comment thread keeper/msg_server_vaults.go
Comment thread simapp/app.yaml
Comment thread keeper/msg_server_vaults_test.go
Comment thread keeper/msg_server_vaults_test.go Outdated
Comment thread keeper/msg_server_vaults_test.go Outdated
Copy link
Copy Markdown
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the review comments so quickly. Great work again! 🙌

@johnletey johnletey merged commit 2ab9e5f into main Jul 30, 2025
6 checks passed
@johnletey johnletey deleted the luca/vault-program-end branch July 30, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants