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.

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

SIP-335 allow approve amount 0 to revoke erc20 #1742

Merged
merged 4 commits into from
Aug 16, 2023
Merged

Conversation

davidvuong
Copy link
Contributor

@davidvuong davidvuong commented Aug 3, 2023

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #1742 (da6cbb1) into main (fa1857c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1742   +/-   ##
=======================================
  Coverage   72.77%   72.77%           
=======================================
  Files          57       57           
  Lines         720      720           
  Branches      236      236           
=======================================
  Hits          524      524           
  Misses        167      167           
  Partials       29       29           
Flag Coverage Δ
core-contracts 93.26% <100.00%> (ø)
core-modules 90.47% <ø> (ø)
core-utils 68.57% <ø> (ø)
Files Changed Coverage Δ
utils/core-contracts/contracts/token/ERC20.sol 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@davidvuong davidvuong marked this pull request as ready for review August 3, 2023 04:26
Copy link
Contributor

@sunnyvempati sunnyvempati left a comment

Choose a reason for hiding this comment

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

pre approving but couple small comments

@@ -175,6 +175,12 @@ contract ERC20 is IERC20 {
}
}

function _checkZeroAddress(address target) private pure {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe reuse this by calling this function in _checkZeroAddressOrAmount. nonblocking

@@ -221,6 +217,20 @@ describe('ERC20', function () {
assertBn.equal(evt.args.amount, approvalAmount);
});

describe('approve()', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

assuming there's already a test for zero address?

@noahlitvin noahlitvin changed the base branch from main to sip-326-332-333-334 August 16, 2023 12:31
@noahlitvin noahlitvin merged commit 5f27305 into sip-326-332-333-334 Aug 16, 2023
19 checks passed
@noahlitvin noahlitvin deleted the SIP-335 branch August 16, 2023 16:47
noahlitvin added a commit that referenced this pull request Aug 30, 2023
* feat: SIP-326, additional pool configuration

* fix: linting issue

* fix: resolve PR comments

* add ability to specify pool collateral configuration

in particular, the ability to limit the amounto f collateral thats deposited to a pool

* ability to pass runtime values into oracle manager nodes

they can be passed as an array of keys and values. a simple loop
looking over all the keys can find any necessary . an example of how this could be used is for an external node to check to see what the
volume of a particular order is, to modify the oracle's resulting value based on the calculation.

* lint fix

* feat: add pool collateral configuration

* feat: update pool collateral configuration

* fix: liniting

* fixes for audit

* fix typo in external node definition
* emit parameter error when bytes32 arrays dont match up

* fix: failing tests

* SIP-332 (#1743)

* started sip-332

* updated IssueUSDModule

* fixing tests

* defined var

* fixed all tests

* removed comment

* fixing tests

* removing unused constant

* building repo

* fixing test

* removed only

* ran build in the root

---------

Co-authored-by: butternutsquash <[email protected]>

* update storage

* feat: update function name

* feat: update mock external node

* fix: linting issue

* adding tests

* update tests

* Q-1 Macro

* Q-3 Macro

* Q-8 Macro

* I-1 Macro

* Spotmarket I-1 Macro

* M-3 removing payable

* SIP-335 allow approve amount 0 to revoke erc20 (#1742)

* SIP-335 allow approve amount 0 to revoke erc20

* Reuse _checkZeroAddress in _checkZeroAddressOrAmount

Addressed comments in PR #1742.

---------

Co-authored-by: Sunny Vempati <[email protected]>

* Sip-323 (#1729)

* feat: upgrade token in AssociatedSystemsModule

* fix test assert

* fix: perps graph issue

* feat: update core subgraph core proxy

* leave symbol for now

---------

Co-authored-by: Noah Litvin <[email protected]>
Co-authored-by: Matías <[email protected]>
Co-authored-by: Matías Lescano <[email protected]>
Co-authored-by: Noisekit <[email protected]>

* SIP-331 Add getMarketAddress view to the MarketManagerModule (#1703)

* Add getMarketAddress view to the MarketManagerModule

* Regenerate docs

* docs

---------

Co-authored-by: Noah Litvin <[email protected]>

* docgen

* fixed tests

* added L-2 Macro

* fix linting

* fixing test

* fixing test

* added test case

* merged main and gen contracts docs

* remove only

* Sip-332: update pool collateral configuration logic (#1776)

* feat: update pool collateral configuration logic

* feat: update pool configuration module

* remove multicallThrough

* increase spot market factory test coverage

* added test case

* removed precision check

* fix: update pool tests

* lint fix

* fix: storage issue

* feat: rename maxDepositD18 to collateralLimitD18

* fix: revert in decreaseAvailableCollateral

* chore: remove redundant code in spot market

* fix: issuanceRatioD18 and liquidationRatioD18 validation

* chore: update error name

* feat: set depositing enabled for USD

---------

Co-authored-by: Daniel Beal <[email protected]>
Co-authored-by: Noah Litvin <[email protected]>
Co-authored-by: max <[email protected]>
Co-authored-by: butternutsquash <[email protected]>
Co-authored-by: max <[email protected]>
Co-authored-by: David Vuong <[email protected]>
Co-authored-by: Sunny Vempati <[email protected]>
Co-authored-by: Matías <[email protected]>
Co-authored-by: Matías Lescano <[email protected]>
Co-authored-by: Noisekit <[email protected]>
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.

3 participants