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

Coburn/integrate poster #10

Merged
merged 4 commits into from
Jun 24, 2019
Merged

Coburn/integrate poster #10

merged 4 commits into from
Jun 24, 2019

Conversation

coburncoburn
Copy link
Contributor

@coburncoburn coburncoburn commented Jun 24, 2019

a few refactors here.

  1. use the web3 timeout option instead of wrapping with p-timeout library, and allow passing timeout as command line argument
  2. switch from env vars to yargs
  3. hack around ganache eth_chainId rpc, there is a ganache PR
    Added eth_chainId trufflesuite/ganache#419
  4. add fixtures for web3 calls
  5. open oracle payload naming changed from "message" to "encoded"

@@ -22,7 +22,10 @@ yarn add open-oracle-poster
To run as a standalone:

```
open-oracle-poster --poster-key=0x...
start reporter
yarn run start --private_key=0x5763aa1cb4c9cd141a1b409d92e5c5b967a28e18c70eb4cd965374ad75bff356 --script="examples/fixed.js"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just some notes for starting integration, room for improvement 4 sure

Copy link
Collaborator

@hayesgm hayesgm left a comment

Choose a reason for hiding this comment

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

looks amazing-- few commentations

poster/README.md Outdated Show resolved Hide resolved
poster/src/index.ts Outdated Show resolved Hide resolved
poster/src/index.ts Show resolved Hide resolved
// afer this timeout
web3.eth.transactionPollingTimeout = argv.timeout;

if (argv.web3_provider === "http://127.0.0.1:8545") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're curious, truffle does an API call to the server and checks to see if evm_snapshot is an API function, and uses that to determine if the server is ganache. I think this is better-- or even just adding a CLI-option (though it would probably trip users up if they didn't read too carefully). So -- in sum -- I like this simple approach to "detect if ganache"-- this might belong in a different file, though.

} else {
throw(e);
throw(e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove semi?

senderKey : string,
viewAddress : string,
functionName : string,
web3 ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

web3: Web should work if you import web3

functionName : string,
web3 ) {
const payloads = await fetchPayloads(sources.split(","));
console.log(payloads)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to keep the console log, maybe format it? Or just for debugging?

}

async function fetchGasPrice() : Promise<number> {
let source = "https://api.compound.finance/api/gas_prices/get_gas_price";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe catch and return a default if this is down?

}

// e.g. findTypes("postPrices(bytes[],bytes[],string[])")-> ["bytes[]","bytes[]","string[]"]
function findTypes(functionName : string) : string[] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, this is going to be fragile-- though it doesn't look like web3 exposes a "parseAbi" function-- which is kind of lame.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least, could be a good case for a regular expression? e.g. (\w+)?\((.*)\) and then return match group 1 (the optional function name) and match group two split by ,?

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 found a library version of this function https://github.com/ethereumjs/ethereumjs-abi/blob/master/lib/index.js#L81, copied it in since it's unexported

// ABI encoded values to be written to the open oracle data contract.
message: string,
encoded: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be changed in many places

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 think I got them

Copy link
Collaborator

@hayesgm hayesgm left a comment

Choose a reason for hiding this comment

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

looks amazing.

@coburncoburn coburncoburn merged commit 632768b into master Jun 24, 2019
@coburncoburn coburncoburn deleted the coburn/integrate-poster branch June 24, 2019 22:25
function buildTrxData(payloads : DelFiReporterPayload[], functionName : string) : string {
const types = findTypes(functionName);

let messages = payloads.map(x => x.encoded);
Copy link
Contributor

Choose a reason for hiding this comment

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

encodeds?

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.

None yet

3 participants