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

chequebook: remove erc20 bindings #1362

Merged
merged 2 commits into from
Mar 15, 2021
Merged

chequebook: remove erc20 bindings #1362

merged 2 commits into from
Mar 15, 2021

Conversation

ralph-pichler
Copy link
Member

this is the third in a series of PRs which remove the use of abigen-generated bindings in favour of just using the few things we need directly building on top of #1361.

This PR

  • removes use of bindings for erc20
  • introduces an erc20 service instead which uses go-ethereum abi directly (this has been extracted as some of its functions are also needed in storage-incentives where some code is currently duplicated)

@ralph-pichler ralph-pichler self-assigned this Mar 1, 2021
@ralph-pichler ralph-pichler force-pushed the factory_no_bindings branch 2 times, most recently from 28828d4 to 7efc2a6 Compare March 3, 2021 17:04
@ralph-pichler ralph-pichler added the builds on open pr builds on another open one label Mar 4, 2021
Base automatically changed from factory_no_bindings to master March 8, 2021 08:16
@ralph-pichler ralph-pichler marked this pull request as ready for review March 8, 2021 08:24
@ralph-pichler ralph-pichler added ready for review The PR is ready to be reviewed and removed builds on open pr builds on another open one labels Mar 8, 2021
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Few minor comments

@@ -138,7 +131,7 @@ func Init(
if err == storage.ErrNotFound {
logger.Info("no chequebook found, deploying new one.")
if swapInitialDeposit.Cmp(big.NewInt(0)) != 0 {
err = checkBalance(ctx, logger, swapInitialDeposit, swapBackend, chainId, overlayEthAddress, erc20BindingFunc, erc20Address, 20*time.Second, 10)
err = checkBalance(ctx, logger, swapInitialDeposit, swapBackend, chainId, overlayEthAddress, erc20Service, 20*time.Second, 10)
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 extract the last two arguments to be declared at the beginning of the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

done. as they are constants I also removed them as arguments.

request := &transaction.TxRequest{
To: &c.address,
Data: callData,
GasPrice: nil,
Copy link
Member

Choose a reason for hiding this comment

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

if the GasPrice is nil and GasLimit is 0 then what happens to the transaction once it gets sent? Not really clear what is the expected behavior here?

Copy link
Member Author

Choose a reason for hiding this comment

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

those are the values for the transaction request, not the transaction itself. gas price nil means the transaction service will query the ethereum node for a gas price, gas limit 0 means it will do a gas estimation.

Copy link
Member

Choose a reason for hiding this comment

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

cool. maybe good to document this inline?

Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

neat

@ralph-pichler
Copy link
Member Author

/run beekeeper

@ralph-pichler ralph-pichler merged commit d7c2d16 into master Mar 15, 2021
@ralph-pichler ralph-pichler deleted the erc20_no_bindings branch March 15, 2021 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants