Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

[FEATURE] Balances|Trustlines|Orders: Return ledger_index, use last validated ledger by default (RLJS-127) #230

Merged
merged 1 commit into from
Dec 5, 2014
Merged

[FEATURE] Balances|Trustlines|Orders: Return ledger_index, use last validated ledger by default (RLJS-127) #230

merged 1 commit into from
Dec 5, 2014

Conversation

alandotcom
Copy link
Contributor

Also, returns validated key as a boolean (from the rippled response)

wltsmrz
wltsmrz reviewed Dec 4, 2014
View reviewed changes
api/balances.js
@@ -77,7 +79,7 @@ function getBalances(request, response, next) {
var accountLinesRequest;
var marker = request.query.marker;
var limit = /^[0-9]*$/.test(request.query.limit) ? Number(request.query.limit) : void(0);
var ledger = /^[0-9]*$/.test(request.query.ledger) ? Number(request.query.ledger) : void(0);
var ledger = /^[0-9]*$/.test(request.query.ledger) ? Number(request.query.ledger) : 'validated';
Copy link
Contributor

Choose a reason for hiding this comment

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

> /^[0-9]*$/.test('') ? Number('') : 'validated'
0

Might want to make it [0-9]+ or Just test !isNaN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks like a bug? I can make that change a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not include it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that as well! I'm still figuring out what the dev practices are for ripple-rest/lib. (Aside: That's technically a change in implementation unrelated to this PR. In my past experience, keeping commits atomic made reverting changes easier if they introduced additional bugs)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would make it work as expected, [0-9]* isn't a good number test. This [FEATURE] doesn't seem complete without a reasonable is-number test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer hiding the implementation details in a function, which can be unit tested. That validator snippet above looks good to me. I'll see if that works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this look like correct behavior?

> validator.isValid('', 'UINT32')
true

> validator.isValid('f', 'UINT32')
false

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently that ain't no good neither

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible that the regex is incorrect?

In the schema file for UINT32:

^$|^(429496729[0-5]|42949672[0-8][0-

That first case is checking for an empty string. @emschwartz do you remember where that schema came from? It comes from cd45dd7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in latest commit, 699c920 (rebased with #231 231)

@alandotcom alandotcom force-pushed the feature/return_ledger_index branch 2 times, most recently from 218bc0d to 017da26 Compare December 5, 2014 00:20
@wltsmrz
Copy link
Contributor

wltsmrz commented Dec 5, 2014

LGTM

@alandotcom
Copy link
Contributor Author

Hold off on merging, I'm going to add this to Orders as welll

@geertweening
Copy link
Contributor

Squash please

@alandotcom
Copy link
Contributor Author

I was going to squash after review

@geertweening
Copy link
Contributor

LGTM

@alandotcom alandotcom force-pushed the feature/return_ledger_index branch from 172820b to 065f30a Compare December 5, 2014 17:53
@alandotcom
Copy link
Contributor Author

Squashed

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 065f30a on lumberj:feature/return_ledger_index into f1989aa on ripple:develop.

@alandotcom alandotcom changed the title [FEATURE] Balances|Trustlines: Return ledger_index, use last validated ledger by default (RLJS-127) [FEATURE] Balances|Trustlines|Orders: Return ledger_index, use last validated ledger by default (RLJS-127) Dec 5, 2014
boxbag
boxbag reviewed Dec 5, 2014
View reviewed changes
test/balances-test.js
conn.send(fixtures.accountLinesResponse(message, {
marker: NEXT_MARKER,
validated: false
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not passing in ledger to response

@alandotcom
Copy link
Contributor Author

There's also a bug I need to address in a followup:

GET http://localhost:5990/v1/accounts/rMwjYedjc7qqtKYVLiAccJSmCwih4LnE2q/balances?currency=XRP

{
  "success": true,
  "balances": [{
    "value": "52070.985931",
    "currency": "XRP",
    "counterparty": ""
  }]
}

The rippled response looks like it's querying against the currently (non-validated) open ledger

boxbag
boxbag reviewed Dec 5, 2014
View reviewed changes
test/trustlines-test.js
@@ -74,9 +77,10 @@ suite('get trustlines', function() {
});

self.wss.once('request_account_lines', function(message, conn) {
console.log(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this console.log

@alandotcom alandotcom force-pushed the feature/return_ledger_index branch from de4dcc7 to 992dd76 Compare December 5, 2014 22:52
@alandotcom
Copy link
Contributor Author

Updated

@alandotcom alandotcom force-pushed the feature/return_ledger_index branch from 992dd76 to 5fa3eb6 Compare December 5, 2014 22:57
@alandotcom
Copy link
Contributor Author

rebased

boxbag
boxbag reviewed Dec 5, 2014
View reviewed changes
test/balances-test.js
.expect(testutils.checkBody(fixtures.RESTAccountBalancesXRPResponse))
.expect(testutils.checkBody(fixtures.RESTAccountBalancesXRPResponse({
ledger: LEDGER,
validated: true
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need validated: true here as it is a default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I'll check the other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@alandotcom alandotcom force-pushed the feature/return_ledger_index branch from 5fa3eb6 to 7d7a8e7 Compare December 5, 2014 23:06
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7d7a8e7 on lumberj:feature/return_ledger_index into 347bf03 on ripple:develop.

@boxbag
Copy link
Contributor

boxbag commented Dec 5, 2014

OK. LGTM, good work @LumberJ

alandotcom
alandotcom reviewed Dec 5, 2014
View reviewed changes
test/fixtures/orders.js
_.defaults(options, {
account: addresses.VALID
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot about this guy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

[FEATURE] Balances|Trustlines|Orders: Return ledger index (RLJS-127)
9bdb0cc 
[FEATURE] Balances|Trustlines: Return ledger index (RLJS-127)

[FEATURE] Balances|Trustlines: Use last validated ledger by default

[FEATURE] Balances|Trustlines: Return ledger validated in response

Returns validated (boolean) if the ledger_index returned is validated or
not

[TASK] Balances|Trustlines: Check limit and ledger are valid uint32 and not empty string

[FEATURE] Orders: Return ledger_index, default to last validated

[TEST] Orders|Balances|Trustlines: Refactor some of the tests

[BUG] Balances: Return XRP balance based on last validated ledger

Also address some comments on PR

[TASK] Trustlines: Remove extra tests, PR comments addressed

[TASK] Orders: Removed unneccesary tests, responding to PR comments
@alandotcom alandotcom force-pushed the feature/return_ledger_index branch from 7d7a8e7 to 9bdb0cc Compare December 5, 2014 23:22
geertweening added a commit that referenced this pull request Dec 5, 2014
@geertweening
Merge pull request #230 from lumberj/feature/return_ledger_index
65be807 
[FEATURE] Balances|Trustlines|Orders: Return ledger_index, use last validated ledger by default (RLJS-127)
@geertweening geertweening merged commit 65be807 into ripple:develop Dec 5, 2014
@alandotcom alandotcom deleted the feature/return_ledger_index branch December 5, 2014 23:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alandotcom @wltsmrz @geertweening @coveralls @boxbag