Add support for vault fee recipients #119
Conversation
…the-sdk # Conflicts: # packages/graphql/src/graphql-env.d.ts # packages/spec/vaults/vault.helpers.ts # packages/spec/vaults/vaults.spec.ts
packages/spec/vaults/vaults.spec.ts
Outdated
| const result = await vaults(client, { | ||
| criteria: { | ||
| ownedBy: [evmAddress(organization.account!.address)], | ||
| }, | ||
| }); | ||
|
|
||
| assertOk(result); | ||
|
|
||
| expect(result.value.items).toEqual([ | ||
| expect.objectContaining({ | ||
| owner: organization.account!.address, | ||
| address: initialVault.value!.address, | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
This part of the test is not needed, we have already tests fetching vault by ownedBy so I would remove this part
There was a problem hiding this comment.
There is a difference on how vault behaves when it has single vs multiple recipients thats why have added the tests and clearly separate them
Given the Aave Vaults
When an organization deploys a new vault
And is single recipient of the vault's fees
Then it should be available in the organization vaults
vs
Given the Aave Vaults
When an organization deploys a new vault
And has multiple recipient of the vault's fees
Then it should be available in the organization vaults
but happy to consider other options, main point is that we need to test this as it has 2 different code paths
There was a problem hiding this comment.
What's the actual assertion here? Simply it does not fail? Can we make this more business oriented and see the revenue flowing to the addresses?
There was a problem hiding this comment.
I think you are already checking the owner in line 87 (creating vault and fetching vault by txHash and later assertion in the owner, that should be enough) so no need to fetch byOwner again.
Also it would be good to check when withdrawing fees that new accounts received the tokens.
There was a problem hiding this comment.
@cesarenaldi the assertion here is that a vault deployed with multi recipients is in the org vaults
@juangm that's already there, it's a separate test (line 588)
There was a problem hiding this comment.
what the owner has to do with the multi-recipient aspect of this feature?
There was a problem hiding this comment.
I agree we should minimize duplication, but if the assertion indeed should be changed to something else (like I am suspecting) then effectively 1 of these 2 tests is not longer redundant with the existing one @juangm is referring to.
packages/spec/vaults/vaults.spec.ts
Outdated
| const result = await vaults(client, { | ||
| criteria: { | ||
| ownedBy: [evmAddress(organization.account!.address)], | ||
| }, | ||
| }); | ||
|
|
||
| assertOk(result); | ||
| expect(result.value.items).toEqual([ | ||
| expect.objectContaining({ | ||
| owner: organization.account!.address, | ||
| address: initialVault.value!.address, | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
Not needed to add this check
packages/spec/vaults/vaults.spec.ts
Outdated
| .andThen(client.waitForTransaction); | ||
| assertOk(updateResult); | ||
|
|
||
| const newVaultInfo = await vault(client, { | ||
| by: { address: initialVault.value.address }, | ||
| chainId: initialVault.value.chainId, | ||
| }); | ||
| assertOk(newVaultInfo); |
There was a problem hiding this comment.
We could still use the .andThen(...
| .andThen(client.waitForTransaction); | |
| assertOk(updateResult); | |
| const newVaultInfo = await vault(client, { | |
| by: { address: initialVault.value.address }, | |
| chainId: initialVault.value.chainId, | |
| }); | |
| assertOk(newVaultInfo); | |
| .andThen(client.waitForTransaction) | |
| .andThen(() => vault(client, { | |
| by: { address: initialVault.value.address }, | |
| chainId: initialVault.value.chainId, | |
| }); | |
| assertOk(updateResult); |
There was a problem hiding this comment.
i feel like andThen should be used to join all related operation into a single final result, this feels like 2 different operations, especially given we use the newVaultInfo inside an assertion
There was a problem hiding this comment.
In other examples we do like operation + fetch updated data. In this case, it would be like: update fees in vault + fetch vault with updated fee
packages/spec/vaults/vaults.spec.ts
Outdated
| .andThen(client.waitForTransaction); | ||
| assertOk(updateResult); | ||
|
|
||
| const newVaultInfo = await vault(client, { | ||
| by: { address: initialVault.value.address }, | ||
| chainId: initialVault.value.chainId, | ||
| }); | ||
| assertOk(newVaultInfo); |
There was a problem hiding this comment.
| .andThen(client.waitForTransaction); | |
| assertOk(updateResult); | |
| const newVaultInfo = await vault(client, { | |
| by: { address: initialVault.value.address }, | |
| chainId: initialVault.value.chainId, | |
| }); | |
| assertOk(newVaultInfo); | |
| .andThen(client.waitForTransaction) | |
| .andThen(() => vault(client, { | |
| by: { address: initialVault.value.address }, | |
| chainId: initialVault.value.chainId, | |
| }); | |
| assertOk(updateResult); |
There was a problem hiding this comment.
i feel like andThen should be used to join all related operation into a single final result, this feels like 2 different operations, especially given we use the newVaultInfo inside an assertion
packages/graphql/schema.graphql
Outdated
| vaultApr: PercentValue! | ||
|
|
||
| """The recipients of the vault revenue""" | ||
| feeRecipients: [RecipientPercent!]! |
There was a problem hiding this comment.
Is 'fee' here used with the same meaning as 'fee' when used in feesBalance for example?
| * @param request - The vault set revenue splitter request parameters. | ||
| * @returns The transaction data for setting a revenue splitter. | ||
| */ | ||
| export function vaultSetRevenueSplitter( |
No description provided.