feat: claim rewards from compound#12707
Conversation
dckc
left a comment
There was a problem hiding this comment.
a couple points I'm not clear on...
| const { addresses: a } = ctx; | ||
| const session = makeEvmAbiCallBatch(); | ||
| const compoundRewardsController = session.makeContract( | ||
| a.compoundRewardsController, |
There was a problem hiding this comment.
Upgrade Considerations
this adds new addresses to the axelar-config, so redepolyment of ymax contract is required with new args
I don't see any changes in axelar-config.
There was a problem hiding this comment.
these contracts were already implemented in the withdraw function. The original behaviour was that rewards claiming could be triggered during a withdraw by adding a claim flag.
Now this functionality has been moved seperately under the claimRewards function
There was a problem hiding this comment.
I guess that means the Upgrade Considerations should change?
There was a problem hiding this comment.
updated. thanks for pointing it out
| ]); | ||
| }); | ||
|
|
||
| test('claim rewards from Compound position', async t => { |
There was a problem hiding this comment.
Testing Considerations
unit tests have been added, but e2e testing has not been performed, since that can only be done in a mainnet environment
a feat usually comes with a contract-level test.
dckc
left a comment
There was a problem hiding this comment.
I'd like to understand the UX better.
| const rebalanceP = trader1.rebalance( | ||
| t, | ||
| { give: { Deposit: amount }, want: {} }, | ||
| { | ||
| flow: [ | ||
| { | ||
| dest: '@Arbitrum', | ||
| src: 'Compound_Arbitrum', | ||
| amount: usdc.make(100n), | ||
| fee: feeCall, | ||
| claim: true, | ||
| }, | ||
| ], | ||
| }, | ||
| ); |
There was a problem hiding this comment.
the trader interface is supposed to be an example of what a front end might use.
Is this how we expect the claim UX to work? It involves a deposit too?
There was a problem hiding this comment.
The only story about non-auto claiming is for an internal user way to manually trigger (as documented in https://linear.app/agoric/issue/AGO-652/let-internal-user-manually-trigger-claim-rewards). Until that is implemented, it is acceptable to just create a rebalance flow, and submit steps that the planner would create if it had identified/created a rewards claim flow.
That said I would not expect to see a deposit amount in a reblance (pretty sure the contract actually drops those)
| }); | ||
|
|
||
| test('claim rewards on Compound position successfully', async t => { | ||
| const { trader1, common, txResolver } = await setupTrader(t); |
There was a problem hiding this comment.
Do we mostly expect this to be used via EVM? Is it straightforward to do an evm trader test?
There was a problem hiding this comment.
It's unclear whether internal user tests would be able to manually trigger rewards claiming through evm wallets. That said it should be possible to write a test where the test planner submits rewards steps.
| /** Discriminant: claim flag routes the step to claimRewards. */ | ||
| claim: true; |
There was a problem hiding this comment.
Should this move to the "base" PR?
8b391af to
6ec75f2
Compare
1511c66 to
16047a9
Compare
16047a9 to
ce1d8ea
Compare
closes: https://linear.app/agoric/issue/PAK-432/ymax-contract-should-be-able-to-claim-reward-tokens-on-compound
Description
claimparam from thewithdrawfunction and instead uses it to trigger theclaimRewardsfunction. so now to trigger claiming rewards for compound, a step like this would be used:Testing Considerations
unit tests have been added, but e2e testing has not been performed, since that can only be done in a mainnet environment
Upgrade Considerations
redepolyment of ymax contract is required for this feature