-
Notifications
You must be signed in to change notification settings - Fork 298
feat(sdk-coin-tao): add moveStakeBuilder #6925
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
base: master
Are you sure you want to change the base?
Conversation
import { assert as SinonAssert, spy } from 'sinon'; | ||
import { MoveStakeBuilder } from '../../../src/lib/moveStakeBuilder'; | ||
import utils from '../../../src/lib/utils'; | ||
import { accounts, mockTssSignature, rawTx, genesisHash, chainName } from '../../resources'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, we should remove the unused import rawTx
from the import statement on line 6 in modules/sdk-coin-tao/test/unit/transactionBuilder/moveStakeBuilder.ts
.
- Specifically, edit the import statement to exclude
rawTx
, retaining the other imports if they are used. - Only the import line needs modification; no further code edits are required unless other unused imports exist (but CodeQL flagged only
rawTx
).
-
Copy modified line R6
@@ -3,7 +3,7 @@ | ||
import { assert as SinonAssert, spy } from 'sinon'; | ||
import { MoveStakeBuilder } from '../../../src/lib/moveStakeBuilder'; | ||
import utils from '../../../src/lib/utils'; | ||
import { accounts, mockTssSignature, rawTx, genesisHash, chainName } from '../../resources'; | ||
import { accounts, mockTssSignature, genesisHash, chainName } from '../../resources'; | ||
import { buildTestConfig } from './base'; | ||
import { testnetMaterial } from '../../../src/resources'; | ||
import { InvalidTransactionError } from '@bitgo/sdk-core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
@claude review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the comments
d196a1b
to
68ad0d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for moving stake between hotkeys in the TAO network by implementing a new moveStakeBuilder
transaction builder.
- Adds
MoveStakeBuilder
andMoveStakeTransaction
classes to handle move stake operations - Extends transaction factory to support move stake transaction type
- Adds comprehensive test coverage for the move stake functionality
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
modules/sdk-coin-tao/test/unit/transactionBuilder/moveStakeBuilder.ts | Comprehensive test suite for MoveStakeBuilder functionality |
modules/sdk-coin-tao/src/lib/transactionBuilderFactory.ts | Adds getMoveStakeBuilder method and factory routing |
modules/sdk-coin-tao/src/lib/moveStakeTransaction.ts | Transaction class for move stake operations |
modules/sdk-coin-tao/src/lib/moveStakeBuilder.ts | Builder class for constructing move stake transactions |
modules/sdk-coin-tao/src/lib/index.ts | Exports new move stake classes |
modules/sdk-coin-tao/src/lib/iface.ts | Adds MoveStakeTxData interface |
modules/sdk-coin-tao/src/lib/constants.ts | Defines TAO network constants |
modules/abstract-substrate/src/lib/utils.ts | Adds isMoveStake utility method |
modules/abstract-substrate/src/lib/txnSchema.ts | Adds validation schema for move stake transactions |
modules/abstract-substrate/src/lib/iface.ts | Adds move stake interfaces and method names |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
explanation.outputs[0].should.deepEqual({ | ||
address: '5Ffp1wJCPu4hzVDTo7XaMLqZSvSadyUQmxWPDw74CBjECSoq', | ||
amount: '9007199254740995', | ||
tokenName: 'ttao:apex', | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded tokenName 'ttao:apex' appears to be a magic value. Consider using a constant or deriving it from the test data to make the test more maintainable and less brittle.
Copilot uses AI. Check for mistakes.
mockBuilder['_method'] = { | ||
name: 'transferKeepAlive', | ||
args: { dest: { id: 'test' }, value: '1000' }, | ||
pallet: 'balances', | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct access to private fields using bracket notation makes the test brittle and violates encapsulation. Consider adding a test-only setter method or refactoring the builder to support this test scenario more cleanly.
Copilot uses AI. Check for mistakes.
private validateNetuid(netuid: string): void { | ||
const netuidNum = Number(netuid); | ||
if (isNaN(netuidNum) || !Number.isInteger(netuidNum) || netuidNum < 0 || netuidNum > TAO_CONSTANTS.MAX_NETUID) { | ||
throw new InvalidTransactionError( | ||
`Invalid netuid: ${netuid}. Netuid must be between 0 and ${TAO_CONSTANTS.MAX_NETUID}.` | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation allows decimal numbers that convert to integers (e.g., '5.0' becomes 5). Use parseInt()
with radix 10 and check if the result equals the original input, or use a regex to ensure the input contains only digits.
Copilot uses AI. Check for mistakes.
Ticket: SC-3017