Skip to content

Supply/Repay on behalf #106

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.

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

Merged
merged 7 commits into from
Jul 14, 2023
Merged

Supply/Repay on behalf #106

merged 7 commits into from
Jul 14, 2023

Conversation

MerlinEgalite
Copy link
Contributor

Fix #28

This PR supposed that we don't use meta tx (still to decide).

@MerlinEgalite MerlinEgalite changed the title Supply/Repay on beahlf Supply/Repay on behalf Jul 11, 2023
@MerlinEgalite MerlinEgalite requested a review from Rubilmax July 11, 2023 09:57
@Rubilmax Rubilmax linked an issue Jul 12, 2023 that may be closed by this pull request
Copy link
Contributor

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a very simple test (for each function) checking that it's working

@MathisGD MathisGD mentioned this pull request Jul 12, 2023
@@ -324,14 +324,37 @@ contract BlueTest is Test {
assertEq(borrowableAsset.balanceOf(address(blue)), amountLent - amountBorrowed + amountRepaid, "blue balance");
}

function testSupplyCollateral(uint256 amount) public {
function testRepayOnBehalf(uint256 amountLent, uint256 amountBorrowed, uint256 amountRepaid, address onBehalf)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is also redundant with testRepay, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some variations, in the test that could be handled if if/else statements but tbh we'll to revamp test anyway, I don't think it's worth spending too much time on this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we might want to make sure that it does not only check that onbehalk works when it is equal to the sender. I'm not sure if it's worth duplicating every test

@MerlinEgalite
Copy link
Contributor Author

I don't get why the CI is failing those lines do not even exist...

@@ -324,14 +324,37 @@ contract BlueTest is Test {
assertEq(borrowableAsset.balanceOf(address(blue)), amountLent - amountBorrowed + amountRepaid, "blue balance");
}

function testSupplyCollateral(uint256 amount) public {
function testRepayOnBehalf(uint256 amountLent, uint256 amountBorrowed, uint256 amountRepaid, address onBehalf)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we might want to make sure that it does not only check that onbehalk works when it is equal to the sender. I'm not sure if it's worth duplicating every test

@Rubilmax
Copy link
Collaborator

I don't get why the CI is failing those lines do not even exist...

Are you sure feat/onbehalf is up-to-date with feat/approval? CI runs on a default merge of the base branch into the head branch.

@MerlinEgalite
Copy link
Contributor Author

I don't get why the CI is failing those lines do not even exist...

Are you sure feat/onbehalf is up-to-date with feat/approval? CI runs on a default merge of the base branch into the head branch.

ah good point... let me check

@MerlinEgalite MerlinEgalite merged commit 370e488 into feat/approval Jul 14, 2023
@MerlinEgalite MerlinEgalite deleted the feat/onbehalf branch July 14, 2023 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust position on behalf of another position
7 participants