Skip to content
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.

Sign up for GitHub

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

Jump to bottom

orchestration - ICA Controller - Distribution - Withdraw Delegator Reward API #9071

Closed
ivanlei opened this issue Mar 11, 2024 · 2 comments · Fixed by #9307
Closed

orchestration - ICA Controller - Distribution - Withdraw Delegator Reward API #9071

ivanlei opened this issue Mar 11, 2024 · 2 comments · Fixed by #9307
Assignees
@dckc
Labels
enhancement New feature or request

Comments

@ivanlei
Copy link
Contributor

ivanlei commented Mar 11, 2024 •
edited by dckc
Loading

What is the Problem Being Solved?

#9065 #8881 describes a generic method for sending ICA commands.

The WithdrawDelegatorReward API should build on that, allowing cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward messages to be sent without the caller needing to build a representation of the message objects.

Description of the Design

Security Considerations

Scaling Considerations

Test Plan

Upgrade Considerations
@ivanlei ivanlei added the enhancement New feature or request label Mar 11, 2024
@dckc dckc mentioned this issue Apr 30, 2024
Merged
@dckc
Copy link
Member

dckc commented Apr 30, 2024 •
edited
Loading

The WithdrawDelegatorReward API should ...

Should I take the name WithdrawDelegatorReward literally? Normally, we use camelCase, following airbnb style.

Also: is it supposed to be a function? or a method on an object?
BaseOrchestrationAccount.withdrawReward looks likely, but I don't see any implementations of BaseOrchestrationAccount yet.

We don't yet have Orchestrator.getChain nor Chain.makeAccount (nee createAccount).
Do we have issues to make those? The closest I can find is

I suppose stakingAccountKit.js has some relevant stuff:

        // TODO move this beneath the Orchestration abstraction,
        // to the OrchestrationAccount provided by createAccount()

https://github.com/Agoric/agoric-sdk/blob/fbd8633ae9f8420a589dd9bc32925418f2dde060/packages/orchestration/src/exos/stakingAccountKit.js#L96C1-L97C67

at @0xpatrickdev 's suggestion, I added a method there in #9307

@dckc
Copy link
Member

dckc commented May 1, 2024 •
edited
Loading

The spec for withdrawRewards says

The promise settles when the rewards are withdrawn.
@returns The total amounts of rewards withdrawn

How do I/we get a signal when the rewards are withdrawn?

p.s. I'll assume a reply from executeEncodedTx means it's done.

@mergify mergify bot closed this as completed in #9307 May 3, 2024
mergify bot added a commit that referenced this issue May 3, 2024
@mergify
feat: withdrawReward on StakingAccountHolder (#9307)
dcca603 
refs: #9071

I'm not confident this closes #9071; see that issue for questions about
scope.

## Description

Adds `WithdrawReward` on `invitationMakers` of `StakingAccountHolder`,
following the pattern set by `Delegate`.

### 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 `WithdrawReward` `MsgSpendAction`
compensates for the cost of the IBC transaction.

### Documentation Considerations

It's not entirely clear to me what the status of `StakingAccountHolder`
is. Is it an example? If so, maybe this is `docs` rather than `feat`?

### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dckc dckc

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: withdrawReward on StakingAccountHolder Agoric/agoric-sdk
2 participants
@dckc @ivanlei