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.

Sign up for GitHub

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

Jump to bottom

Revert with error message when release validation fails #37

Merged
merged 3 commits into from
Jul 24, 2018
Merged

Revert with error message when release validation fails #37

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69d7b94
Add validation error codes / revert on error
cgewecke Jul 16, 2018
6581c05
Update tests to check for revert & error msgs
cgewecke Jul 16, 2018
61e356a
Revert from validateRelease and update tests
cgewecke Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 16 additions & 13 deletions contracts/PackageIndex.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,19 +124,22 @@ contract PackageIndex is Authorized, PackageIndexInterface {
);
}

if (!releaseValidator.validateRelease(
packageDb,
releaseDb,
msg.sender,
name,
majorMinorPatch,
preRelease,
build,
releaseLockfileURI)
)
{
// Release is invalid
return false;
// Run release validator. This method returns a non-zero integer code
// if the release cannot proceed.
uint8 code = releaseValidator.validateRelease(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of returning the code, the revert should just be thrown from within the releaseValidator? That way you don't have to juggle a return code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pipermerriam Agree - that's a more elegant solution and works perfectly for the case where you're attempting a release and have an error.

There's a small tradeoff - if you call validateRelease on its own - say as a pre-flight check - the errors don't propagate because it's a view function. It can be structured to correctly return true/false for that case though.

Your suggestion is simpler/cleaner (probably lighter) ... on reflection definitely prefer.

@gnidan WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

So right now the benefit is that errors propagate easily via eth_call...

To continue to get errors to propagate, we could just leave it as a client concern (say, sendTransaction to the view function on a forked network). But this is messy and heavy-handed in implementation...

It does make it a lot simpler in Solidity, to revert with errors inside ReleaseValidator. To get a pre-flight check we'd need another method, though, one that doesn't error opaquely.

I'm not sure. I don't feel strongly either way, but I guess my vote is that pre-flight checks are valuable enough to warrant providing that capability in some form, even if it means a bit of ugly code somewhere. But I'll abstain from voting on where the ugly code to support this should live.

Copy link
Member

Choose a reason for hiding this comment

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

TLDR: Do what you think is best.

Lately I've structured my APIs to look something like this:

is_valid_thing()  # returns `true/false`
validate_thing()  # throws error

I find I almost always want both of these and it is easy to make one a wrapper around the other. In this case it isn't trivial because we don't have good error handling so you might need a 3rd internal method which returns a status code which can then be converted to either a boolean, or an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pipermerriam Yes, makes sense. For now opting to revert from the validator per suggestion because its cleaner/less weird. Will possibly add a second is_valid_thing -like method that returns error string or "ok" for pre-flight checks in a later PR. After the revisions in 61e356a the behavior is:

  • reverts with error message on send
  • returns true/false on call

packageDb,
releaseDb,
msg.sender,
name,
majorMinorPatch,
preRelease,
build,
releaseLockfileURI
);

// Revert with error message if release did not validate.
if (code != 0){
revert(releaseValidator.errors(code));
}

// Compute hashes
Expand Down
40 changes: 29 additions & 11 deletions contracts/ReleaseValidator.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,21 @@ import {ReleaseDB} from "./ReleaseDB.sol";
/// @title Database contract for a package index.
/// @author Piper Merriam <[email protected]>
contract ReleaseValidator {

/// @dev Maps integer error codes returned by `validateRelease` to human readable error strings.
mapping (uint8 => string) public errors;

/// @dev Initializes `errors` map with human readable error messages.
constructor() public {
errors[1] = "escape:ReleaseValidator:package-db-not-set";
errors[2] = "escape:ReleaseValidator:release-db-not-set";
errors[3] = "escape:ReleaseValidator:caller-not-authorized";
errors[4] = "escape:ReleaseValidator:version-exists";
errors[5] = "escape:ReleaseValidator:invalid-package-name";
errors[6] = "escape:ReleaseValidator:invalid-lockfile-uri";
errors[7] = "escape:ReleaseValidator:invalid-release-version";
}

/// @dev Runs validation on all of the data needed for releasing a package. Returns success.
/// @param packageDb The address of the PackageDB
/// @param releaseDb The address of the ReleaseDB
Expand All @@ -28,28 +43,31 @@ contract ReleaseValidator {
)
public
view
returns (bool)
returns (uint8 code)
{
require(address(packageDb) != 0x0, "escape:ReleaseValidator:package-db-not-set");
require(address(releaseDb) != 0x0, "escape:ReleaseValidator:release-db-not-set");

if (!validateAuthorization(packageDb, callerAddress, name)) {
if (address(packageDb) == 0x0){
// packageDb address is null
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

These error numbers seem like they should be an Enum. That would make reading this code a lot easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes nice! 🙂

} else if (address(releaseDb) == 0x0){
// releaseDb address is null
return 2;
} else if (!validateAuthorization(packageDb, callerAddress, name)) {
// package exists and msg.sender is not the owner not the package owner.
return false;
return 3;
} else if (!validateIsNewRelease(packageDb, releaseDb, name, majorMinorPatch, preRelease, build)) {
// this version has already been released.
return false;
return 4;
} else if (!validatePackageName(packageDb, name)) {
// invalid package name.
return false;
return 5;
} else if (!validateReleaseLockfileURI(releaseLockfileURI)) {
// disallow empty release lockfile URI
return false;
return 6;
} else if (!validateReleaseVersion(majorMinorPatch)) {
// disallow version 0.0.0
return false;
return 7;
}
return true;
return 0;
}

/// @dev Validate whether the callerAddress is authorized to make this release.
Expand Down
124 changes: 83 additions & 41 deletions test/releaseValidator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ contract('ReleaseValidator', function(accounts){
packageIndex,
packageDB,
releaseDB,
reason
){
const nameHash = await packageDB.hashName(info[0])
const versionHash = await releaseDB.hashVersion(...info.slice(1, -1));
Expand All @@ -62,7 +63,10 @@ contract('ReleaseValidator', function(accounts){
info.pop();
info.push(otherUri);

await packageIndex.release(...info);
await assertFailure(
packageIndex.release(...info),
reason
);

packageData = await packageIndex.getPackageData(info[0]);
releaseData = await packageIndex.getReleaseData(releaseHash);
Expand All @@ -88,7 +92,10 @@ contract('ReleaseValidator', function(accounts){
const info = releases.zero;
assert( await packageIndex.packageExists(info[0]) === false );

await packageIndex.release(...info);
await assertFailure(
packageIndex.release(...info),
'invalid-release-version'
);

assert( await packageIndex.packageExists(info[0]) === false );
assert( await packageIndex.releaseExists(...info.slice(0,-1)) === false);
Expand All @@ -99,7 +106,8 @@ contract('ReleaseValidator', function(accounts){
releases.zeroMinor,
packageIndex,
packageDB,
releaseDB
releaseDB,
'version-exists',
);
});

Expand All @@ -108,7 +116,8 @@ contract('ReleaseValidator', function(accounts){
releases.zeroPatch,
packageIndex,
packageDB,
releaseDB
releaseDB,
'version-exists',
);
});

Expand All @@ -117,7 +126,8 @@ contract('ReleaseValidator', function(accounts){
releases.major,
packageIndex,
packageDB,
releaseDB
releaseDB,
'version-exists',
);
});

Expand All @@ -126,7 +136,8 @@ contract('ReleaseValidator', function(accounts){
releases.minor,
packageIndex,
packageDB,
releaseDB
releaseDB,
'version-exists',
);
});

Expand All @@ -135,7 +146,8 @@ contract('ReleaseValidator', function(accounts){
releases.patch,
packageIndex,
packageDB,
releaseDB
releaseDB,
'version-exists',
);
});

Expand All @@ -144,7 +156,8 @@ contract('ReleaseValidator', function(accounts){
releases.prerelease,
packageIndex,
packageDB,
releaseDB
releaseDB,
'version-exists',
);
});
});
Expand Down Expand Up @@ -407,11 +420,16 @@ contract('ReleaseValidator', function(accounts){
assert( await packageIndex.packageExists(name) === true);
}

async function assertDoesNotRelease(packageIndex, info){
async function assertDoesNotRelease(packageIndex, info, reason){
const name = info[0];

assert( await packageIndex.packageExists(name) === false);
await packageIndex.release(...info);

await assertFailure(
packageIndex.release(...info),
reason
);

assert( await packageIndex.packageExists(name) === false);
}

Expand Down Expand Up @@ -445,52 +463,52 @@ contract('ReleaseValidator', function(accounts){
describe('invalid', function(){
it('a', async function(){
info[0] = cases.a;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
});

it('a * 215', async function(){
info[0] = cases.a215;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})

it('-starts-with-dash', async function(){
info[0] = cases.startsDash;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})

it('9starts-with-number', async function(){
info[0] = cases.startsNumber;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})

it('hasCapitals', async function(){
info[0] = cases.hasCaps;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})

it('Starts-with-capital', async function(){
info[0] = cases.startsCaps;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})

it('with_underscore', async function(){
info[0] = cases.underscore;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})

it('with.period', async function(){
info[0] = cases.period;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})

it('with$money-symbol', async function(){
info[0] = cases.dollar;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})

it('with()parenthesis', async function(){
info[0] = cases.parens;
await assertDoesNotRelease(packageIndex, info);
await assertDoesNotRelease(packageIndex, info, 'invalid-package-name');
})
});
});
Expand All @@ -501,40 +519,59 @@ contract('ReleaseValidator', function(accounts){
const info = ['test', 1, 0, 0, '', '', ''];

assert( await packageIndex.packageExists('test') === false );
await packageIndex.release(...info);

await assertFailure(
packageIndex.release(...info),
'invalid-lockfile-uri'
);

assert( await packageIndex.packageExists('test') === false );
})
});

describe('Existence of dependent DB', function(){
it('throws on release if the PackageDB has been unset', async function(){
const infoA = ['test', 1, 0, 0, '', '', 'ipfs://some-ipfs-uri'];
const infoB = ['test', 1, 1, 0, '', '', 'ipfs://some-ipfs-uri'];
describe('validateRelease checks existence of DBs', function(){
it('returns error code 1 if PackageDB param is null', async function(){
const info = ['test', 1, 0, 0, '', '', 'ipfs://some-ipfs-uri'];

assert( await packageIndex.packageExists('test') === false );
await packageIndex.release(...infoA);
assert( await packageIndex.packageExists('test') === true );

await packageIndex.setPackageDb(helpers.zeroAddress);
await assertFailure(
packageIndex.release(...infoB),
'package-db-not-set'
const code = await releaseValidator.validateRelease(
helpers.zeroAddress,
releaseDB.address,
accounts[0],
info[0],
info.slice(1,4),
info[4],
info[5],
info[6]
);

const message = await releaseValidator.errors(code);

assert(code === '1');
assert(message.includes('package-db-not-set'));
});

it('throws on release if the ReleaseDB has been unset', async function(){
const infoA = ['test', 1, 0, 0, '', '', 'ipfs://some-ipfs-uri'];
const infoB = ['test', 1, 1, 0, '', '', 'ipfs://some-ipfs-uri'];
it('returns error code 2 if ReleaseDB is null', async function(){
const info = ['test', 1, 0, 0, '', '', 'ipfs://some-ipfs-uri'];

assert( await packageIndex.packageExists('test') === false );
await packageIndex.release(...infoA);
assert( await packageIndex.packageExists('test') === true );

await packageIndex.setReleaseDb(helpers.zeroAddress);
await assertFailure(
packageIndex.release(...infoB),
'release-db-not-set'
const code = await releaseValidator.validateRelease(
packageDB.address,
helpers.zeroAddress,
accounts[0],
info[0],
info.slice(1,4),
info[4],
info[5],
info[6]
);

const message = await releaseValidator.errors(code);

assert(code === '2');
assert(message.includes('release-db-not-set'));
});
});

Expand All @@ -555,7 +592,12 @@ contract('ReleaseValidator', function(accounts){
assert(data.packageOwner === newOwner);

assert( await packageIndex.releaseExists(...infoB.slice(0, -1)) === false );
await packageIndex.release(...infoB, {from: nonOwner})

await assertFailure(
packageIndex.release(...infoB, {from: nonOwner}),
'caller-not-authorized'
);

assert( await packageIndex.releaseExists(...infoB.slice(0, -1)) === false );
});
})
Expand Down