feat(stnear): adding support for metapool withdraw_all txn builder#8199
feat(stnear): adding support for metapool withdraw_all txn builder#8199aditya-bitgo merged 1 commit intomasterfrom
Conversation
73b4f34 to
4307e81
Compare
There was a problem hiding this comment.
MetaPoolWithdrawBuilder duplicates significant portions of StakingWithdrawBuilder (constructor, initBuilder, gas, buildImplementation). Consider extending the existing builder with a validateArgs hook -- see inline suggestion.
explainStakingWithdrawTransaction returns outputAmount: '0' for withdraw_all since the amount is resolved on-chain. Acceptable trade-off, but downstream consumers should be aware.
Factory routing, transaction type mapping, and input/output handling are all correct. LIQUID_STAKING addition to stnear tokens is appropriate.
Tests are comprehensive: signed/unsigned builds, round-trips, routing verification, error case. LGTM.
09960e7 to
a67f81e
Compare
Doddanna17
left a comment
There was a problem hiding this comment.
Hey @aditya-bitgo, the refactoring looks clean -- the 17-line MetaPoolWithdrawBuilder extending StakingWithdrawBuilder with the validateArgs hook is exactly the right pattern.
One small thing: MetaPoolWithdrawBuilder inherits the public amount() method from StakingWithdrawBuilder. Since validateArgs is a no-op here, calling .amount('...') on a MetaPoolWithdrawBuilder would silently set args on a withdraw_all call without any error. Nothing calls it today, but an override that throws would prevent accidental misuse:
public amount(_amount: string): this {
throw new BuildTransactionError('amount is not applicable for withdraw_all');
}Rest looks solid. LGTM with that minor addition.
a764212 to
7fc604f
Compare
Doddanna17
left a comment
There was a problem hiding this comment.
All feedback addressed -- amount() override with throw, validateArgs parameter signature, and test for the rejection case. Clean inheritance pattern, comprehensive tests. LGTM.
ticket: SC-5580