Skip to content
This repository has been archived by the owner on Nov 24, 2021. It is now read-only.

Commit

Permalink
Throw an error when Amount or Fee contains a decimal (Fix #31)
Browse files Browse the repository at this point in the history 
Thanks to @jwbusch for review
  • Loading branch information
@intelliot
intelliot authored Jul 26, 2019
1 parent aa943d3 commit baf2b44
Show file tree
Hide file tree
Showing 15 changed files with 413 additions and 162 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"devDependencies": {
"babel-cli": "^6.8.0",
"babel-core": "^6.8.0",
"babel-eslint": "^6.0.4",
"babel-eslint": "^10.0.2",
"babel-loader": "^6.2.4",
"babel-preset-es2015": "^6.6.0",
"babel-register": "^6.8.0",
Expand Down
2 changes: 1 addition & 1 deletion src/serdes/binary-parser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ const BinaryParser = makeClass({
const value = kls.fromParser(this, sizeHint);
if (value === undefined) {
throw new Error(
`fromParser for (${field.name}, ${field.type.name}) -> undefined `);
`fromParser for (${field.name}, ${field.type.name}) -> undefined `);
}
return value;
},
Expand Down
4 changes: 2 additions & 2 deletions src/types/account-id.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ const AccountID = makeClass({
statics: {
from(value) {
return value instanceof this ? value :
/^r/.test(value) ? this.fromBase58(value) :
new this(value);
/^r/.test(value) ? this.fromBase58(value) :
new this(value);
},
cache: {},
fromCache(base58) {
Expand Down
30 changes: 24 additions & 6 deletions src/types/amount.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ function raiseIllegalAmountError(value) {

const parsers = {
string(str) {
if (!str.match(/\d+/)) {
// Using /^\d+$/ here fixes #31
if (!str.match(/^\d+$/)) {

This comment has been minimized.

Sign in to view

Copy link
@sublimator

sublimator Jul 27, 2019

Contributor

ugh!

This comment has been minimized.

Sign in to view

Copy link
@sublimator

sublimator Jul 27, 2019

Contributor

I think this was down to some confusion about match semantics, thinking that match meant that the whole string matched, i.e. '^' and '$' implied.

I think the '^' is implied, but not '$'

This comment has been minimized.

Sign in to view

Copy link
@sublimator

sublimator Jul 27, 2019

Contributor

In general this library doesn't do much validation, at it was originally intended for something internal and most the validation was done by RippleAPI. There's likely other nasties lurking.

#10
raiseIllegalAmountError(str);
}
return [new Decimal(str).dividedBy(DROPS_PER_XRP), Currency.XRP];
Expand All @@ -66,8 +67,8 @@ const parsers = {
assert(isDefined(object.currency), 'currency must be defined');
assert(isDefined(object.issuer), 'issuer must be defined');
return [new Decimal(object.value),
Currency.from(object.currency),
AccountID.from(object.issuer)];
Currency.from(object.currency),
AccountID.from(object.issuer)];
}
};

Expand Down Expand Up @@ -109,7 +110,7 @@ const Amount = makeClass({
mantissa[1] &= 0x3F;
// decimal.js won't accept e notation with hex
const value = new Decimal(`${sign}0x${bytesToHex(mantissa)}`)
.times('1e' + exponent);
.times('1e' + exponent);
return new this(value, currency, issuer, false);
}

Expand All @@ -128,6 +129,7 @@ const Amount = makeClass({
// value is in XRP scale, but show the value in canonical json form
raiseIllegalAmountError(this.value.times(DROPS_PER_XRP))
}
this.verifyNoDecimal(this.value); // This is a secondary fix for #31
} else {
const p = this.value.precision();
const e = this.exponent();
Expand All @@ -143,8 +145,24 @@ const Amount = makeClass({
return this.currency.isNative();
},
mantissa() {
// This is a tertiary fix for #31
const integerNumberString = this.verifyNoDecimal();

return new UInt64(
new BN(this.value.times('1e' + -this.exponent()).abs().toString()));
new BN(integerNumberString));
},
verifyNoDecimal() {
const integerNumberString = this.value
.times('1e' + -this.exponent()).abs().toString();
// Ensure that the value (after being multiplied by the exponent)
// does not contain a decimal. From the bn.js README:
// "decimals are not supported in this library."
// eslint-disable-next-line max-len
// https:/indutny/bn.js/blob/9cb459f044853b46615464eea1a3ddfc7006463b/README.md
if (integerNumberString.indexOf('.') !== -1) {
raiseIllegalAmountError(integerNumberString);
}
return integerNumberString;
},
isZero() {
return this.value.isZero();
Expand All @@ -154,7 +172,7 @@ const Amount = makeClass({
},
valueString() {
return (this.isNative() ? this.value.times(DROPS_PER_XRP) : this.value)
.toString();
.toString();
},
toBytesSink(sink) {
const isNative = this.isNative();
Expand Down
2 changes: 1 addition & 1 deletion src/types/hash.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const Hash = makeClass({
Hash(bytes) {
const width = this.constructor.width;
this._bytes = bytes ? parseBytes(bytes, Uint8Array) :
new Uint8Array(width);
new Uint8Array(width);
assert.equal(this._bytes.length, width);
},
mixins: [Comparable, SerializedType],
Expand Down
6 changes: 3 additions & 3 deletions src/types/uint.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const UInt = makeClass({
width: 0,
fromParser(parser) {
const val = this.width > 4 ? parser.read(this.width) :
parser.readUIntN(this.width);
parser.readUIntN(this.width);
return new this(val);
},
from(val) {
Expand All @@ -40,8 +40,8 @@ const UInt = makeClass({
const otherValue = other.valueOf();
if (thisValue instanceof BN) {
return otherValue instanceof BN ?
thisValue.cmp(otherValue) :
thisValue.cmpn(otherValue);
thisValue.cmp(otherValue) :
thisValue.cmpn(otherValue);
} else if (otherValue instanceof BN) {
return -other.compareTo(this);
}
Expand Down
22 changes: 11 additions & 11 deletions test/binary-json-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ describe('ripple-binary-codec', function() {
entries.forEach((t, test_n) => {
// eslint-disable-next-line max-len
it(`${name}[${test_n}] can encode ${truncateForDisplay(json(t.json))} to ${truncateForDisplay(t.binary)}`,
() => {
assert.equal(t.binary, encode(t.json));
});
() => {
assert.equal(t.binary, encode(t.json));
});
// eslint-disable-next-line max-len
it(`${name}[${test_n}] can decode ${truncateForDisplay(t.binary)} to ${truncateForDisplay(json(t.json))}`,
() => {
const decoded = decode(t.binary);
assert.deepEqual(t.json, decoded);
});
() => {
const decoded = decode(t.binary);
assert.deepEqual(t.json, decoded);
});
});
});
}
Expand All @@ -34,10 +34,10 @@ describe('ripple-binary-codec', function() {
describe('ledgerData', function() {
fixtures.ledgerData.forEach((t, test_n) => {
it(`ledgerData[${test_n}] can decode ${t.binary} to ${json(t.json)}`,
() => {
const decoded = decodeLedgerData(t.binary);
assert.deepEqual(t.json, decoded);
});
() => {
const decoded = decodeLedgerData(t.binary);
assert.deepEqual(t.json, decoded);
});
});
})
});
70 changes: 35 additions & 35 deletions test/binary-parser-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ function transactionParsingTests() {
'SigningPubKey':
'028472865AF4CB32AA285834B57576B7290AA8C31B459047DB27E16F418D6A7166',
'TakerGets': {'currency': 'ILS',
'issuer': 'rNPRNzBB92BVpAhhZr4iXDTveCgV5Pofm9',
'value': '1694.768'},
'issuer': 'rNPRNzBB92BVpAhhZr4iXDTveCgV5Pofm9',
'value': '1694.768'},
'TakerPays': '98957503520',
'TransactionType': 'OfferCreate',
'TxnSignature': __(`
Expand Down Expand Up @@ -88,7 +88,7 @@ function transactionParsingTests() {
// amount currency
assert(Hash160.fromParser(parser));
assert.equal(encodeAccountID(parser.read(20)),
tx_json.TakerGets.issuer);
tx_json.TakerGets.issuer);
assert.equal(parser.readField(), Field.Fee);
assert(parser.read(8));
assert.equal(parser.readField(), Field.SigningPubKey);
Expand Down Expand Up @@ -209,8 +209,8 @@ function assertRecyclable(json, forField) {
const sink = new BytesList();
Type.from(recycled).toBytesSink(sink);
const recycledAgain = makeParser(sink.toHex())
.readType(Type)
.toJSON();
.readType(Type)
.toJSON();
assert.deepEqual(recycledAgain, json);
}

Expand Down Expand Up @@ -285,36 +285,36 @@ function pathSetBinaryTests() {

const expectedJSON =
[[{account: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K',
currency: 'BTC',
issuer: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K'},
{account: 'rM1oqKtfh1zgjdAgbFmaRm3btfGBX25xVo',
currency: 'BTC',
issuer: 'rM1oqKtfh1zgjdAgbFmaRm3btfGBX25xVo'},
{account: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B',
currency: 'BTC',
issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'},
{currency: 'USD',
issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'}],
[{account: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K',
currency: 'BTC',
issuer: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K'},
{account: 'rM1oqKtfh1zgjdAgbFmaRm3btfGBX25xVo',
currency: 'BTC',
issuer: 'rM1oqKtfh1zgjdAgbFmaRm3btfGBX25xVo'},
{account: 'rpvfJ4mR6QQAeogpXEKnuyGBx8mYCSnYZi',
currency: 'BTC',
issuer: 'rpvfJ4mR6QQAeogpXEKnuyGBx8mYCSnYZi'},
{currency: 'USD',
issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'}],
[{account: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K',
currency: 'BTC',
issuer: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K'},
{account: 'r3AWbdp2jQLXLywJypdoNwVSvr81xs3uhn',
currency: 'BTC',
issuer: 'r3AWbdp2jQLXLywJypdoNwVSvr81xs3uhn'},
{currency: '0000000000000000000000005852500000000000'},
{currency: 'USD',
issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'}]];
currency: 'BTC',
issuer: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K'},
{account: 'rM1oqKtfh1zgjdAgbFmaRm3btfGBX25xVo',
currency: 'BTC',
issuer: 'rM1oqKtfh1zgjdAgbFmaRm3btfGBX25xVo'},
{account: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B',
currency: 'BTC',
issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'},
{currency: 'USD',
issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'}],
[{account: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K',
currency: 'BTC',
issuer: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K'},
{account: 'rM1oqKtfh1zgjdAgbFmaRm3btfGBX25xVo',
currency: 'BTC',
issuer: 'rM1oqKtfh1zgjdAgbFmaRm3btfGBX25xVo'},
{account: 'rpvfJ4mR6QQAeogpXEKnuyGBx8mYCSnYZi',
currency: 'BTC',
issuer: 'rpvfJ4mR6QQAeogpXEKnuyGBx8mYCSnYZi'},
{currency: 'USD',
issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'}],
[{account: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K',
currency: 'BTC',
issuer: 'r9hEDb4xBGRfBCcX3E4FirDWQBAYtpxC8K'},
{account: 'r3AWbdp2jQLXLywJypdoNwVSvr81xs3uhn',
currency: 'BTC',
issuer: 'r3AWbdp2jQLXLywJypdoNwVSvr81xs3uhn'},
{currency: '0000000000000000000000005852500000000000'},
{currency: 'USD',
issuer: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'}]];

it('works with long paths', () => {
const parser = makeParser(bytes);
Expand Down
4 changes: 2 additions & 2 deletions test/binary-serializer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ function nestedObjectTests() {
fixtures.whole_objects.forEach((f, i) => {
it(`whole_objects[${i}]: can parse blob and dump out same blob`,
/* */ () => {
assertRecycles(f.blob_with_no_signing);
});
assertRecycles(f.blob_with_no_signing);
});
});
}

Expand Down
4 changes: 2 additions & 2 deletions test/hash-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('Hash256', function() {
});
it('supports getting the nibblet values at given positions', function() {
const h = Hash256.from(
'1359BD0000000000000000000000000000000000000000000000000000000000');
'1359BD0000000000000000000000000000000000000000000000000000000000');
assert.equal(h.nibblet(0), 0x1);
assert.equal(h.nibblet(1), 0x3);
assert.equal(h.nibblet(2), 0x5);
Expand All @@ -53,6 +53,6 @@ describe('Currency', function() {
assert.throws(() => Currency.from(new Uint8Array(19)));
assert.throws(() => Currency.from(1));
assert.throws(() => Currency.from(
'00000000000000000000000000000000000000m'));
'00000000000000000000000000000000000000m'));
});
});
4 changes: 2 additions & 2 deletions test/ledger-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ describe('Ledger Hashes', function() {
const ledger = loadFixture(ledgerFixture);
it('computes correct account state hash', function() {
assert.equal(accountStateHash(ledger.accountState).toHex(),
ledger.account_hash);
ledger.account_hash);
});
it('computes correct transaction tree hash', function() {
assert.equal(transactionTreeHash(ledger.transactions).toHex(),
ledger.transaction_hash);
ledger.transaction_hash);
});
it('computes correct ledger header hash', function() {
assert.equal(ledgerHash(ledger).toHex(), ledger.hash);
Expand Down
Loading

0 comments on commit baf2b44

Please sign in to comment.