Updates on Spend Reporting PR 3087 #3131

MSevey · 2018-06-29T22:32:26Z

Removed old contracts from Export to avoid breaking backwards compatibility.

Updated releaseblock calculation.

@DavidVorick if there are other edits you find during your review please add TODO's to this PR so we can merge them in.

DavidVorick · 2018-07-02T17:49:27Z

And you need to update the api documentation for the /renter/contracts endpoint to reflect that old contracts are now returned as well.

And... we should consider having the old contracts get returned as a flag instead of always getting returned, because over time that will build up to be a bunch of data.

MSevey · 2018-07-02T18:33:59Z

And... we should consider having the old contracts get returned as a flag instead of always getting returned, because over time that will build up to be a bunch of data.

Are you saying to look for the flag in renterContractsHandler so that only the active contracts are returned unless the flag is provided? So adding like an oldContracts bool or something?

MSevey · 2018-07-02T18:46:33Z

I also went ahead and renamed OldContracts to ExpiredContracts since active and expired make more sense than active and old.

DavidVorick · 2018-07-02T19:36:44Z

Yeah, add a bool so that it only returns the expired contract if a flag is passed in, that way long running renters don't have to worry about getting 50,000 returned objects when they make a query.

DavidVorick · 2018-07-02T21:08:36Z

cmd/siac/rentercmd.go

@@ -475,11 +477,11 @@ func rentercontractscmd() {
 // rentercontractsviewcmd is the handler for the command `siac renter contracts <id>`.
 // It lists details of a specific contract.
 func rentercontractsviewcmd(cid string) {
-	rc, err := httpClient.RenterContractsGet()
+	rc, err := httpClient.RenterContractsGet(true)


since it's not obvious at all what true means in the context of this function, convention is to use a variable:

getAllContracts := true rc, err := httpCliend.RenterContractsGet(getAllContracts)

DavidVorick · 2018-07-02T21:08:46Z

cmd/siac/export.go

@@ -30,13 +30,12 @@ var (
 // renterexportcontracttxnscmd is the handler for the command `siac renter export contract-txns`.
 // Exports the current contract set to JSON.
 func renterexportcontracttxnscmd(destination string) {
-	cs, err := httpClient.RenterContractsGet()
+	cs, err := httpClient.RenterContractsGet(false)


since it's not obvious at all what true means in the context of this function, convention is to use a variable:

getAllContracts := true rc, err := httpCliend.RenterContractsGet(getAllContracts)

DavidVorick · 2018-07-03T19:22:27Z

That distinction looks good to me. inactive would include contracts that have been renewed due to depletion? (I think it would, as they would be !goodForRewew)

…for contracts summary

…cts are needed

…flags

…point was called

…nt period

cmd/siac/rentercmd.go

 	if renterAllContracts {
-		if len(rc.OldContracts) == 0 {
+		rc, err := httpClient.RenterExpiredContractsGet()


cmd/siac/rentercmd.go


-	for _, rc := range contracts {
+	for _, rc := range rc.Contracts {


modules/renter/renter.go

@@ -113,7 +113,7 @@ type hostContractor interface {
 	// Contracts returns the active contracts formed by the contractor.
 	Contracts() []modules.RenterContract

-	// Contracts returns the old contracts formed by the contractor.
+	// OldContracts returns the Expired contracts formed by the contractor.


node/api/renter.go

+//
+// Inactive contracts are contracts that are not currently being used by the
+// renter because they are either !goodForUpload or !goodForRenew, but have
+// endheights that are in the future so could potentially become active again


siatest/renter/renter_test.go

@@ -1339,64 +1383,219 @@ func TestRenterOldContracts(t *testing.T) {
 	}
 	tg, err := siatest.NewGroupFromTemplate(groupParams)
 	if err != nil {
-		t.Fatal("Failed to create group: ", err)
+		t.Fatal("Failed to get files")


siatest/renter/renter_test.go

-// TestRenterResetAllowance tests that resetting the allowance after the
-// allowance was cancelled will trigger the correct contract formation.
+// TestRenterCancelAllowance tests that setting an empty allowance causes
+// uploads, downloads, and renewals to cease.
 func TestRenterResetAllowance(t *testing.T) {


siatest/renter/renter_test.go

@@ -1668,6 +1870,7 @@ func TestRenterSpendingReporting(t *testing.T) {
 	}

 	// Get renter's initial siacoin balance
+	r := tg.Renters()[0]


siatest/renter/renter_test.go

@@ -2016,8 +2251,6 @@ func TestRenterSpendingReporting(t *testing.T) {
 	}
 }

-// The following are helper functions for the renter tests
-
 // checkBalanceVsSpending checks the renters confirmed siacoin balance in their


MSevey · 2018-07-10T19:56:58Z

test-long is passing locally

MSevey · 2018-07-11T14:26:13Z

siac renter contracts

MSevey · 2018-07-11T14:26:39Z

siac renter contracts -A

MSevey · 2018-07-11T15:23:51Z

siac if no contracts have been formed

MSevey · 2018-07-11T15:47:25Z

fixed column headers

lukechampine · 2018-07-12T21:57:10Z

node/api/renter.go

+			if expired && c.EndHeight < blockHeight {
+				expiredContracts = append(expiredContracts, contract)
+			} else if inactive && c.EndHeight >= blockHeight {
+				inactiveContracts = append(inactiveContracts, contract)


Shouldn't this be two separate ifs, not an else if? What if you want both expired and inactive?

no contract will be both inactive and expired, this is fine. We should add a comment though.

MSevey force-pushed the 3087-fixes branch from 2810981 to b4a5cdc Compare July 2, 2018 17:38

MSevey force-pushed the 3087-fixes branch 3 times, most recently from 7e0da14 to 9315ef1 Compare July 2, 2018 19:25

MSevey force-pushed the 3087-fixes branch from dcb517b to b3534f2 Compare July 2, 2018 20:59

DavidVorick reviewed Jul 2, 2018

View reviewed changes

MSevey force-pushed the 3087-fixes branch from b3534f2 to 15a0290 Compare July 2, 2018 21:16

This comment has been minimized.

Sign in to view

MSevey added this to the v1.3.4 milestone Jul 3, 2018

MSevey added the v1.3.4 label Jul 3, 2018

MSevey added 8 commits July 3, 2018 18:54

Remove old contracts from export

721168b

Update ReleaseBlock to be blockheight and not windowsize

3594c51

Change OldContracts to ExpiredContracs, address format issue in siac …

3d727b0

…for contracts summary

Add expired flag to contracts endpoint to determine if expired contra…

e751c09

…cts are needed

Change contracts endpoint to have active, inactive, expired, and all …

e29684a

…flags

Update siatest renter package based on new contracts api endpoints

a9aec9b

Finish updates to siatest and updates to code where contracts api end…

694af1b

…point was called

Debug renter siatests

3dc7ab5

This comment has been minimized.

Sign in to view

MSevey added 5 commits July 9, 2018 12:08

Preparing for rebase

e565a92

Debug failing siatest renter tests

792f9e7

Change contract api endpoint to look at block height instead of curre…

417dad9

…nt period

More siatest renter clean up from changes to contract api endpoint

2894946

Add information to comment on contract api endpoint

66ab5c2

MSevey added 2 commits July 10, 2018 14:02

Update export and siac for api changes to contracts

db75e9d

Update comments on Client api functions

1398eb0

MSevey force-pushed the 3087-fixes branch from 63087ee to 1398eb0 Compare July 10, 2018 18:04

Clean up siatest renter to reduce api calls and error messages

9dbc6eb

MSevey commented Jul 10, 2018

View reviewed changes

cmd/siac/rentercmd.go Outdated

if renterAllContracts {

if len(rc.OldContracts) == 0 {

rc, err := httpClient.RenterExpiredContractsGet()

This comment was marked as resolved.

Sign in to view

MSevey commented Jul 10, 2018

View reviewed changes

cmd/siac/rentercmd.go Outdated

for _, rc := range contracts {

for _, rc := range rc.Contracts {

This comment was marked as resolved.

Sign in to view

MSevey commented Jul 10, 2018

View reviewed changes

siatest/renter/renter_test.go Outdated

@@ -1668,6 +1870,7 @@ func TestRenterSpendingReporting(t *testing.T) {

}

// Get renter's initial siacoin balance

r := tg.Renters()[0]

This comment was marked as resolved.

Sign in to view

MSevey commented Jul 10, 2018

View reviewed changes

MSevey force-pushed the 3087-fixes branch from 2913438 to f69e37c Compare July 10, 2018 19:51

MSevey force-pushed the 3087-fixes branch 2 times, most recently from 7e36187 to a1536e4 Compare July 10, 2018 22:03

Updates from personal review of PR

1b74628

MSevey force-pushed the 3087-fixes branch from a1536e4 to 1b74628 Compare July 11, 2018 13:40

Fix siac bug

665b8d1

Debug NDF for TestRenterSpendingReporting

3ab6201

MSevey force-pushed the 3087-fixes branch from e967f0e to 3ab6201 Compare July 11, 2018 15:21

Fix siac formatting

cc81c96

MSevey mentioned this pull request Jul 11, 2018

Update initial current period to be set in the past when allowance is created #3157

Open

lukechampine reviewed Jul 12, 2018

View reviewed changes

lukechampine merged commit 9f8d535 into master Jul 12, 2018

MSevey deleted the 3087-fixes branch July 16, 2018 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates on Spend Reporting PR 3087 #3131

Updates on Spend Reporting PR 3087 #3131

MSevey commented Jun 29, 2018

DavidVorick commented Jul 2, 2018

MSevey commented Jul 2, 2018

MSevey commented Jul 2, 2018

DavidVorick commented Jul 2, 2018

DavidVorick Jul 2, 2018

DavidVorick Jul 2, 2018

This comment has been minimized.

DavidVorick commented Jul 3, 2018

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

MSevey commented Jul 10, 2018

MSevey commented Jul 11, 2018

MSevey commented Jul 11, 2018

MSevey commented Jul 11, 2018

MSevey commented Jul 11, 2018

lukechampine Jul 12, 2018

DavidVorick Jul 12, 2018


		for _, rc := range contracts {
		for _, rc := range rc.Contracts {

Updates on Spend Reporting PR 3087 #3131

Updates on Spend Reporting PR 3087 #3131

Conversation

MSevey commented Jun 29, 2018

DavidVorick commented Jul 2, 2018

MSevey commented Jul 2, 2018

MSevey commented Jul 2, 2018

DavidVorick commented Jul 2, 2018

DavidVorick Jul 2, 2018

Choose a reason for hiding this comment

DavidVorick Jul 2, 2018

Choose a reason for hiding this comment

This comment has been minimized.

DavidVorick commented Jul 3, 2018

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

MSevey commented Jul 10, 2018

MSevey commented Jul 11, 2018

MSevey commented Jul 11, 2018

MSevey commented Jul 11, 2018

MSevey commented Jul 11, 2018

lukechampine Jul 12, 2018

Choose a reason for hiding this comment

DavidVorick Jul 12, 2018

Choose a reason for hiding this comment