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

fedekunze/1548 mocked submit deposit #1549

Merged
merged 12 commits into from
Nov 13, 2018
Merged

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Nov 11, 2018

Closes #1548

Description:

The only test case missing is when the user already has a deposit to the proposal, but not on the same coin denomination, I can add it to the scope of #1553 :

if (!prevDepCoin) {
   prevDeposit.amount.push(coin)
}

❤️ Thank you!


  • Added entries in CHANGELOG.md with issue # and GitHub username
  • Reviewed Files changed in the github PR explorer

@codecov
Copy link

codecov bot commented Nov 12, 2018

Codecov Report

Merging #1549 into develop will increase coverage by 0.02%.
The diff coverage is 97.67%.

@@             Coverage Diff             @@
##           develop    #1549      +/-   ##
===========================================
+ Coverage    96.52%   96.55%   +0.02%     
===========================================
  Files           99       99              
  Lines         1872     1915      +43     
  Branches        92       92              
===========================================
+ Hits          1807     1849      +42     
- Misses          55       56       +1     
  Partials        10       10
Impacted Files Coverage Δ
app/src/renderer/connectors/lcdClient.js 97.56% <ø> (ø) ⬆️
app/src/renderer/connectors/lcdClientMock.js 99.12% <97.67%> (-0.34%) ⬇️

@fedekunze fedekunze changed the title WIP: fedekunze/1548 mocked submit deposit fedekunze/1548 mocked submit deposit Nov 12, 2018
@@ -353,13 +355,13 @@ let state = {
description: `custom text proposal description`,
initial_deposit: [
{
denom: `stake`,
denom: `steak`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

didn't we want to move towards stake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but it's not going to be used until GoS and the denom for governance is still steak

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, but this is just the mock implementation and not the real network. does it conflict with settings for the real network?

Copy link
Contributor Author

@fedekunze fedekunze Nov 13, 2018

Choose a reason for hiding this comment

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

not really, but I'd have to add another proposal or refactor it to change that, bc it'll probably break other unit tests that use the mocked proposals. Can I do that on the scope of #1378 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure. I just did that PR. Let's see afterwards what breaks. 💥

@faboweb faboweb merged commit 77047dd into develop Nov 13, 2018
@faboweb faboweb deleted the fedekunze/1548-mock-add-deposit branch November 13, 2018 11:23
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.

add submit deposit to lcdClientMock
2 participants