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

Migrating ethwrite adapter to monorepo #290

Merged
merged 17 commits into from
Feb 22, 2021
Merged

Migrating ethwrite adapter to monorepo #290

merged 17 commits into from
Feb 22, 2021

Conversation

ebarakos
Copy link
Contributor

@ebarakos ebarakos commented Feb 3, 2021 •
edited

@ebarakos ebarakos changed the title Migrating ethwrite adapter in monorepo Migrating ethwrite adapter to monorepo Feb 3, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2021

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@ebarakos ebarakos force-pushed the feature/176643310-ethwrite-monorepo-migration branch 5 times, most recently from 6d5453f to bda1d1f Compare February 8, 2021 10:58
@ebarakos ebarakos marked this pull request as ready for review February 8, 2021 16:03
@ebarakos ebarakos force-pushed the feature/176643310-ethwrite-monorepo-migration branch 3 times, most recently from 4ca6233 to c166c8a Compare February 11, 2021 18:31
@ebarakos ebarakos force-pushed the feature/176643310-ethwrite-monorepo-migration branch from c166c8a to 1e3f4e7 Compare February 15, 2021 13:52
RodrigoAD
RodrigoAD reviewed Feb 15, 2021
View reviewed changes
ethwrite/deploy_contract.ts Outdated Show resolved Hide resolved
ethwrite/deploy_contract.ts Outdated Show resolved Hide resolved
ethwrite/read_contract.ts Outdated Show resolved Hide resolved
ethwrite/src/config.ts Outdated Show resolved Hide resolved
ethwrite/src/endpoint/txsend.ts Outdated Show resolved Hide resolved
ethwrite/src/endpoint/txsend.ts
export const execute: ExecuteWithConfig<Config> = async (request) => {
const validator = new Validator(request, customParams)
if (validator.error) throw validator.error

Copy link
Member

Choose a reason for hiding this comment

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

Load the provider and the wallet here, with the values from config

ethwrite/src/endpoint/txsend.ts Outdated Show resolved Hide resolved
ethwrite/src/endpoint/txsend.ts Outdated
if (validator.error) throw validator.error

const jobRunID = validator.validated.id
const externalAddress = validator.validated.data.exAddr || ''
Copy link
Member

Choose a reason for hiding this comment

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

exAddr is mandatory, so no need of giving the default value

ethwrite/deploy_contract.ts Outdated Show resolved Hide resolved
ethwrite/test/adapter.test.ts
import { deploy } from '../deploy_contract'

// using DELAYED ROOT SUITE in order to start the chain and deploy the contract
setTimeout(async function () {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this approach. Could a before work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spent some time to make it working with before, but async initialization does not work on describe/context(mochajs/mocha#2975).

Async calls could be resolved inside it though.

Copy link
Member

Choose a reason for hiding this comment

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

Opened a low priority issue #321 just so we remember this

@ebarakos ebarakos force-pushed the feature/176643310-ethwrite-monorepo-migration branch 4 times, most recently from 6005db1 to ab33a03 Compare February 16, 2021 16:21
boxhock
boxhock reviewed Feb 16, 2021
View reviewed changes
ethwrite/README.md Outdated

- `exAddr`: The address for sending the transaction to.
- `funcId`: (Optional) One of the following setter functions: `0xc2b12a73` for `bytes32`, `0xa53b1c1e` for `int256` or `0xd2282dc5` for `uint256` (defaults to `0xd2282dc5`)
- `dataType`: (Optional) One of the following types: `bytes32`, `int256` or `uint256` (defaults to `uint256`)
Copy link
Contributor

Choose a reason for hiding this comment

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

We normally get the data pre-encoded from the previous tasks in the job run, and IMO we should not have dataType as a param (it should be assumed it's already encoded)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it optional(default usage will not encode the result), to retain the other functionality as well.

@ebarakos ebarakos force-pushed the feature/176643310-ethwrite-monorepo-migration branch from ab33a03 to bcc535b Compare February 16, 2021 17:02
@ebarakos ebarakos force-pushed the feature/176643310-ethwrite-monorepo-migration branch 2 times, most recently from fc7671e to b2cbfc4 Compare February 17, 2021 16:16
RodrigoAD
RodrigoAD reviewed Feb 17, 2021
View reviewed changes
test-helpers/src/hardhat.ts Show resolved Hide resolved
hardhat.config.js Outdated Show resolved Hide resolved
ethwrite/test/adapter.test.ts Outdated Show resolved Hide resolved
ethwrite/src/contract_helpers.ts Outdated Show resolved Hide resolved
ethwrite/src/config.ts Outdated Show resolved Hide resolved
ethwrite/src/config.ts
rpcUrl: string
network?: string
privateKey: string
api: any
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove api right?

Copy link
Contributor Author

@ebarakos ebarakos Feb 18, 2021 •
edited

Choose a reason for hiding this comment

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

If I want to use ExecuteWithConfig<C extends Config>, I can't, since it is required in main Config type. And I don't feel like changing the adapter.ts boilerplate, since IMHO should be removed at some point.

I was thinking of changing this type to ExecuteWithConfig<C> but I am considering of checking this again separately, maybe as part of #293.

I can also just use execute = async (input: AdapterRequest, config: Config): Promise<AdapterResponse> for the particular adapter as in synth-index though. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

You're right, the tweak is made is some places is defining the makeExecute as const makeExecute = (config?: Config): Execute => {...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's check changing it to either ExecuteWithConfig or removing the requirement of api param, in another PR, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it as it is. There's an issue open on Config inflexibility, we can leave it for another PR

Copy link
Member

Choose a reason for hiding this comment

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

Tagging #217

ethwrite/README.md Outdated
}
```

## Deploy & Test
Copy link
Member

Choose a reason for hiding this comment

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

Update the Readme as well. This won't be necessary anymore

ethwrite/test/adapter.test.ts Outdated
const contract = new ethers.Contract(address, abi, provider)

describe('execute', async () => {
const execute = makeExecute({ rpcUrl, privateKey, api: {} })
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

@ebarakos ebarakos force-pushed the feature/176643310-ethwrite-monorepo-migration branch 3 times, most recently from 8636560 to 1065b87 Compare February 19, 2021 14:08
RodrigoAD
RodrigoAD reviewed Feb 19, 2021
View reviewed changes
hardhat.config.js Outdated
// See (https://hardhat.org/config/) for more details.

// const { MNEMONIC } = require('./test.env')
// const MNEMONIC = 'novel mobile inform nurse circle spoon cricket soup crowd clip hawk glad'
Copy link
Member

Choose a reason for hiding this comment

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

Can we clean this comments?

RodrigoAD
RodrigoAD reviewed Feb 19, 2021
View reviewed changes
Copy link
Member

@RodrigoAD RodrigoAD left a comment

Choose a reason for hiding this comment

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

I think only that last changes are needed

@justinkaseman justinkaseman mentioned this pull request Feb 19, 2021
Closed
justinkaseman
justinkaseman previously approved these changes Feb 19, 2021
View reviewed changes
Copy link
Member

@justinkaseman justinkaseman left a comment

Choose a reason for hiding this comment

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

Great work 🥇. LGTM

@ebarakos ebarakos force-pushed the feature/176643310-ethwrite-monorepo-migration branch from 2338cdd to 5255bd4 Compare February 22, 2021 08:47
@ebarakos ebarakos merged commit e25f817 into develop Feb 22, 2021
@ebarakos ebarakos deleted the feature/176643310-ethwrite-monorepo-migration branch February 22, 2021 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boxhock boxhock boxhock left review comments

@RodrigoAD RodrigoAD RodrigoAD approved these changes

@justinkaseman justinkaseman justinkaseman left review comments

@krebernisak krebernisak Awaiting requested review from krebernisak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ebarakos @boxhock @RodrigoAD @justinkaseman