-
Notifications
You must be signed in to change notification settings - Fork 302
feat(flrp): add TxBuilder for P-chain staking #8063
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
Conversation
modules/sdk-coin-flrp/src/lib/permissionlessDelegatorTxBuilder.ts
Outdated
Show resolved
Hide resolved
Vijay-Jagannathan
left a comment
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.
few comments
modules/sdk-coin-flrp/src/lib/permissionlessDelegatorTxBuilder.ts
Outdated
Show resolved
Hide resolved
modules/sdk-coin-flrp/src/lib/permissionlessDelegatorTxBuilder.ts
Outdated
Show resolved
Hide resolved
Vijay-Jagannathan
left a comment
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.
lgtm
abhijit0943
left a comment
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.
Hey Sakshi, nice work on the builder — the overall structure looks solid. One issue though:
transaction.ts wasn't updated in this PR. The setTransactionType() method (line 379) only allows AddPermissionlessValidator in its whitelist, so when the delegation builder calls setTransactionType(AddPermissionlessDelegator) during buildImplementation(), it'll throw "Transaction type is not supported". The outputs, changeOutputs, inputs, and explainTransaction getters also need cases for the delegation tx type.
The reason the existing tests don't catch this is that every build() test expects a rejection before it ever gets to setTransactionType (missing nodeID, missing UTXOs, etc.). There's no test that actually builds a complete valid delegation transaction end-to-end.
Would be good to:
- Update
transaction.tsto supportAddPermissionlessDelegatorinsetTransactionTypeand the output/input/explain getters - Add the delegation type to the
Txunion iniface.ts(it was added toSerializedTxbut notTx) - Add an end-to-end build test with all required params, and ideally an
initBuilderround-trip test (build -> serialize ->factory.from()-> verify)
abhijit0943
left a comment
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.
Looks good — all the issues from the previous review are addressed. transaction.ts is updated, Tx type union is fixed, and the round-trip tests with real tx data cover the deserialization path. Ship it.
TICKET: SC-5233
Implement transaction builder for AddPermissionlessDelegator transactions on Flare P-chain, enabling stake delegation to validators.
Key changes:
Note: Rewards always go to the C-chain address derived from the delegator's public key though it does not affect onchain reward distribution and rewardAddress param is kept for rewards tracking