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

Infura based validator #188

Merged
merged 6 commits into from
Aug 8, 2018
Merged

Infura based validator #188

merged 6 commits into from
Aug 8, 2018

Conversation

mirathewhite
Copy link
Contributor

Uses Infura to query Ethereum mainnet. The validation script queries the deployed proxy contract to verify that stored data is correct. See ./validate/README.validator.md for setup details.

@mirathewhite mirathewhite requested a review from o-a-hudson August 8, 2018 00:34
console.log(name + ": \t" + actual + "\t" + compare(expected, actual));
}

async function Validate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this prints out the actual and expected values? Might be worth doing the comparison and printing out success/failure?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I see, print does the compare, nevermind

@eztierney eztierney merged commit fdf8107 into circlefin:master Aug 8, 2018
var PAUSED = false

// Name of current implementation artifact as stored in ./build/contracts/*.json
var FiatToken = artifacts.require("FiatToken");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an old version of the contract. We should be using "FiatTokenV1" instead of "FiatToken".
To ensure you use the correct version of the contract, I would recommend running "truffle compile --all" before executing this script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix in CENT-251.

@@ -22,6 +37,17 @@ module.exports = {
port: 8545,
network_id: "*" // Match any network id
}
// INFURA Setup
Copy link
Contributor

Choose a reason for hiding this comment

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

It is necessary to have the lines commented out? I think we should be able to have multiple different networks defined but only use the ones we wish for a particular task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Infura requires HDWalletProvider which is not part of the default install. @eztierney and I couldnt figure out a reasonable way to update the yarn.lock file, so I commented out all code related to new packages. We will need to figure out a new yarn.lock workflow.

@@ -0,0 +1,144 @@
// Address of the FiatToken Implementation
var fiatTokenAddress = "0x0882477e7895bdc5cea7cb1552ed914ab157fe56";
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to combine all of these contract variables with expected contract state to be defined in a single javascript object. That way, it's easy to see where to update variable values for when we need to validate the contract against a different set of expected state variables. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CENT-252

}

function getAddressFromSlotData(slotData) {
const rawAddress = slotData.substring(26, 86);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this generally true, that the address is always stored in the substring(26,86) section? Wondering if this will work for all of our potential cases with different variable values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends what other data is packed into the slot. It is a 32 byte slot and the address takes up 20 bytes. If there are other variables (e.g. bools, uint8), they may be before or after the address, depending on the order in which they are declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CENT-252

var blacklister = await proxiedToken.blacklister.call();
print("blacklister", blacklister, BLACKLISTER);

await printMinterInfo(proxiedToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should add a printBlacklistedInfo as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CENT-252


// initialized needs to retrieved manually
var slot8Data = await asyncGetStorageAt(proxiedToken.address, 8);
var initialized = slot8Data.substring(24,26);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this substring(24,26) correct? I thought we were looking for initialized = "0x" + slot8Data.substring(2,4); // first 2 hex-chars after 0x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CENT-252

}

async function Validate() {
console.log("Connecting to contract...");
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this function should return an error code if any comparison fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CENT-252

@o-a-hudson
Copy link
Contributor

Looks like this was merged while I was reviewing. Perhaps not worth fixing comments.

@mirathewhite mirathewhite deleted the blockchainVerification branch August 8, 2018 13:29
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.

3 participants