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

Reserve spend limits should not apply to CP-DOTO #3172

Merged
merged 9 commits into from Mar 31, 2020
Merged

Reserve spend limits should not apply to CP-DOTO #3172

merged 9 commits into from Mar 31, 2020

Conversation

asaj
Copy link
Contributor

@asaj asaj commented Mar 24, 2020

Description

This PR removes the restriction on reserve spending from transfers initiated on behalf of the stability protocol.

Tested

Unit tests.

Other changes

None

Related issues

None

Backwards compatibility

Yes

@asaj asaj requested a review from m-chrzan as a code owner March 24, 2020 14:58
Copy link
Contributor

@aslawson aslawson left a comment

Choose a reason for hiding this comment

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

can you add details in the description?

I'm not sure what CP-DOTO stands for, but the transferExchangeGold method looks like it can only be accessed by registered contracts and the _transferGold internally, so seems to be a safe change that allows only specified contracts to bypass a daily spending limit

LGTM

@codecov
Copy link

codecov bot commented Mar 25, 2020

Codecov Report

Merging #3172 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3172      +/-   ##
==========================================
- Coverage   75.64%   75.62%   -0.02%     
==========================================
  Files         610      609       -1     
  Lines       15302    15275      -27     
  Branches     1901     1899       -2     
==========================================
- Hits        11575    11552      -23     
+ Misses       3391     3387       -4     
  Partials      336      336
Flag Coverage Δ
#mobile 75.74% <ø> (ø) ⬆️
#web 75.46% <ø> (-0.04%) ⬇️
Impacted Files Coverage Δ
packages/web/src/brandkit/KeyImagery.tsx 77.27% <0%> (-0.51%) ⬇️
packages/web/src/brandkit/IconsPage.tsx 90% <0%> (-0.48%) ⬇️
packages/web/src/brandkit/Typography.tsx 84.84% <0%> (-0.45%) ⬇️
packages/web/src/brandkit/common/Showcase.tsx 92.3% <0%> (ø) ⬆️
packages/web/src/shared/InlineAnchor.tsx 100% <0%> (ø) ⬆️
packages/web/src/brandkit/logo/LogoExample.tsx 100% <0%> (ø) ⬆️
packages/web/src/brandkit/tracking.ts
packages/web/src/brandkit/Logo.tsx 100% <0%> (+2.77%) ⬆️
packages/web/src/brandkit/Intro.tsx 100% <0%> (+5.26%) ⬆️
...ackages/web/src/brandkit/common/DownloadButton.tsx 100% <0%> (+8.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 637c08c...ba72138. Read the comment docs.

@jo-tm
Copy link
Contributor

jo-tm commented Mar 25, 2020

CP-DOTO is the Exchange.sol, mainly around exchange().

Copy link
Collaborator

@rcroessmann rcroessmann left a comment

Choose a reason for hiding this comment

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

LGTM!

@jo-tm
Copy link
Contributor

jo-tm commented Mar 25, 2020

Yes, is removing the daily spend limit from transferExchangeGold() that is called by CP-DOTO. Daily spend limit only checked for Custody transferGold() to otherAddresses.

@asaj asaj merged commit bd51ef2 into master Mar 31, 2020
@asaj asaj deleted the asaj/exchange branch March 31, 2020 19:48
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.

None yet

4 participants