Skip to content

feat(sdk-coin-sol): StakingActivate for jito #6501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

haritkapadia
Copy link
Contributor

@haritkapadia haritkapadia commented Jul 21, 2025

TICKET: SC-2314

@haritkapadia haritkapadia force-pushed the harit/SC-2314 branch 4 times, most recently from 212683d to 634a453 Compare July 25, 2025 22:13
package.json Outdated
@@ -128,6 +128,7 @@
"test:prepare-release": "mocha --require ts-node/register ./scripts/tests/prepareRelease/prepare-release-main.test.ts"
},
"dependencies": {
"@solana/spl-stake-pool": "^1.1.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we add the sol dependency under sol module instead?

Comment on lines 89 to +118
isMarinade?: boolean;
isJito?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

use enum stakingMethod for default, marinade and jito instead to keep it clean


constructor(_coinConfig: Readonly<CoinConfig>) {
super(_coinConfig);
this._isTestnet = this._coinConfig.network.type === NetworkType.TESTNET;
Copy link
Contributor

Choose a reason for hiding this comment

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

In other coins, we typically do not have this flag. It's caller's responsibility to decide mainnet or testnet configs to use.

import { coins } from '@bitgo/statics'
import { Connection, PublicKey, clusterApiUrl, Transaction, Keypair, LAMPORTS_PER_SOL } from "@solana/web3.js"
import { getStakePoolAccount, updateStakePool } from '@solana/spl-stake-pool'
import { BinTools } from 'avalanche';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why import avax lib>

@@ -0,0 +1,119 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the directory name is future?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this is helper lib? If yes, then jitoStakePoolOperations.ts might be a less confusing name

@evanzbitgo
Copy link
Contributor

This is reviews from AI

🚨 Critical Issues That Need Immediate Attention

  1. PR Documentation & Process
    Missing PR Description: Your PR only has the ticket number. Please fill out the complete PR template with:
    Summary of changes and motivation
    Type of change (new feature)
    Testing approach and results
    Complete the checklist items
    Draft Status: Remember to mark as ready for review when complete
  2. Architecture & Design Concerns
    Dependency Management Issue ⚠️
    Diff
  • "@solana/spl-stake-pool": "^1.1.8",
    You've added this dependency to the root package.json, but it should be in the modules/sdk-coin-sol/package.json instead. Dependencies should be scoped to the modules that actually use them.

Interface Design Needs Improvement 🔄
TypeScript
export interface StakingActivate {
// ... existing fields
isMarinade?: boolean;

  • isJito?: boolean;
    }
    Problem: Having multiple boolean flags (isMarinade, isJito) will become unwieldy as you add more staking providers.

Better Approach: Use an enum for cleaner, more maintainable code:

TypeScript
export enum StakingMethod {
DEFAULT = 'default',
MARINADE = 'marinade',
JITO = 'jito'
}

export interface StakingActivate {
// ... existing fields
stakingMethod?: StakingMethod;
}
3. Code Quality Issues
Unnecessary Testnet Flag ❌
TypeScript
protected _isTestnet: boolean;

constructor(_coinConfig: Readonly) {
super(_coinConfig);

  • this._isTestnet = this._coinConfig.network.type === NetworkType.TESTNET;
    }
    Issue: In BitGo's architecture, we don't typically store network type flags in builders. The caller should pass the appropriate config for mainnet/testnet. This violates our established patterns.

Incorrect Import ❌
TypeScript
import { BinTools } from 'avalanche';
Issue: You're importing Avalanche utilities in a Solana staking example. This appears to be a copy-paste error and needs to be removed.

Poor Naming & Organization 📁
Directory name future/: This is confusing. What does "future" mean here?
File name depositSol.ts: If this is for Jito stake pool operations, name it clearly like jitoStakePoolOperations.ts
📝 Detailed Code Review Comments
File Structure & Organization
Examples directory: The examples/ts/sol/stake-jito.ts file should demonstrate real-world usage
Helper utilities: Move Jito-specific logic into well-named, focused modules
Test coverage: I don't see corresponding test files for the new functionality
Code Quality Standards
Error handling: Ensure all async operations have proper error handling
Documentation: Add comprehensive JSDoc comments for new public methods
Type safety: Make sure all new interfaces and types are properly typed
Validation: Add input validation for stake amounts, validator addresses, etc.
🧪 Testing Requirements
For a feature this significant, you need:

Unit tests for the new StakingActivateBuilder functionality
Integration tests for Jito staking flow
Tests for both mainnet and testnet scenarios
Error case testing (invalid validators, insufficient funds, etc.)
📋 Action Items for Next Iteration
High Priority
✅ Move @solana/spl-stake-pool dependency to correct package.json
✅ Replace boolean flags with StakingMethod enum
✅ Remove _isTestnet flag and related logic
✅ Remove Avalanche import from Solana example
✅ Rename future/depositSol.ts to something meaningful
Medium Priority
✅ Complete PR description with full template
✅ Add comprehensive test coverage
✅ Add JSDoc documentation
✅ Improve error handling throughout
Before Final Review
✅ Self-review against BitGo coding standards
✅ Ensure all existing tests still pass
✅ Test in both browser and Node environments
✅ Mark PR as ready for review
💡 Learning Opportunities
This is a great learning exercise in:

Interface design: How to design extensible APIs that won't need breaking changes
Dependency management: Understanding monorepo package boundaries
Architecture patterns: Following established patterns in large codebases
Testing strategies: Building confidence through comprehensive test coverage

@haritkapadia
Copy link
Contributor Author

Moved to #6502

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.

2 participants