feat: withdrawReward on StakingAccountHolder #9307
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
aa79cd5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4bf3f91f.agoric-sdk.pages.dev |
| Branch Preview URL: | https://dc-dist-helper.agoric-sdk.pages.dev |
dckc
changed the title
dckc
changed the title
dckc
force-pushed
the
dc-dist-helper
branch
2 times, most recently
from
f421a92
to
a550852
Compare
refs: #9307 ## Description Simplify codegen exports using [exports subpath patterns](https://nodejs.org/api/packages.html#subpath-patterns) Reviewers, - should the exported path have `.js`? - should Agoric exports be under `agoric` like the others? ### Security Considerations <!-- Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls? --> ### Scaling Considerations <!-- Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated? --> ### Documentation Considerations <!-- Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users? --> ### Testing Considerations <!-- Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet? --> ### Upgrade Considerations <!-- What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed? -->
| * @param {Any} x | ||
| * @returns {AnyJson} | ||
| */ | ||
| const toJSON = x => /** @type {AnyJson} */ (Any.toJSON(x)); |
There was a problem hiding this comment.
consider including "Any" in the fn name
| const toJSON = x => /** @type {AnyJson} */ (Any.toJSON(x)); | |
| const toAnyJson = x => /** @type {AnyJson} */ (Any.toJSON(x)); |
| */ | ||
| async withdrawReward(validator) { | ||
| const { chainAddress } = this.state; | ||
| const msg0 = MsgWithdrawDelegatorReward.toProtoMsg({ |
There was a problem hiding this comment.
the 0 is distracting. consider removing or making a more explicit message name
| }; | ||
|
|
||
| /** | ||
| * XXX defined in codegen/cosmos/base/v1beta1/coin.d.ts |
There was a problem hiding this comment.
it is now with the subpath pattern
|
Can we also update or add a test in |
| } catch (cause) { | ||
| throw assert.error(`bad response: ${ackStr}`, undefined, { cause }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
nb: would be nice to see this pulled out of this file as it seems reusable. This seems like it would be consumer facing for folks implementing custom messages.
| @@ -39,16 +43,44 @@ const { Fail } = assert; | |||
| const HolderI = M.interface('holder', { | |||
| getPublicTopics: M.call().returns(TopicsRecordShape), | |||
| makeDelegateInvitation: M.call(M.string(), AmountShape).returns(M.promise()), | |||
| makeWithdrawRewardInvitation: M.call(M.string()).returns(M.promise()), | |||
There was a problem hiding this comment.
would be nice to add withdrawReward to this interface as well
There was a problem hiding this comment.
seems like a lot of redundancy: we have each method on 3 facets - twice on the holder facet if we keep that pattern up. Plus all the type declarations and interface guards.
Maybe the methods like makeDelegateInvitation on holder can just go away, since we have Delegate on the invitationMakers facet.
There was a problem hiding this comment.
given the positive feedback: 8b8f5bd
It's slimmer: +56 -86
| return { account, calls }; | ||
| }; | ||
|
|
||
| const mockZCF = () => { |
There was a problem hiding this comment.
Curious, there's nothing we could import here?
There was a problem hiding this comment.
maybe there is, but I'm not familiar with it.
In this case of build-vs-buy, it seemed easier to write a few lines than to get into the car to go shopping.
There was a problem hiding this comment.
I looked around a bit; I don't see anything in zoe/tools.
The closest I found is zoe/test/unitTests/zcf/setupZcfTest.js, but that has a hard-coded contract.
- test: withdrawReward on StakingAccountHolder.helpers facet - test tx encoding layers - test non-trivial delegations - factor out tryDecodeResponse, toJSON - todos for remaining work
- implement delegate() etc. directly on holder;
- avoids unguarded helper methods
- since these methods don't have multiple callers,
it makes little sense to put them on the helper facet anyway
- call zcf.makeInvitation() directly from invitationMaker facet
methods; prune makeXInvitation methods on holder
refs: #9071
I'm not confident this closes #9071; see that issue for questions about scope.
Description
Adds
WithdrawRewardoninvitationMakersofStakingAccountHolder, following the pattern set byDelegate.Security Considerations
I'm a bit uneasy about lack of guards for the helper facet (
helper: UnguardedHelperI). It seems to impose a non-local review burden: we have to be sure every call to the helper facet passes only args that have already been guarded. I tripped on it, once: I passed the wrong type of thing to a helper method and didn't get a helpful guard violation message.Scaling Considerations
While proportionally, the amount of work done doesn't outgrow the message size (presuming an IBC transaction is O(1)), in absolute terms, it seems unlikely that the fee for a
WithdrawRewardMsgSpendActioncompensates for the cost of the IBC transaction.Documentation Considerations
It's not entirely clear to me what the status of
StakingAccountHolderis. Is it an example? If so, maybe this isdocsrather thanfeat?Testing Considerations
Based on internal discussions, the tests here are unit tests that mock the rest of the system.
Upgrade Considerations
This presumably gets deployed with the rest of orchestration in an upcoming chain-halting upgrade.