Skip to content

Adopt sendTransaction Logic From Client Library #206

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

Closed
orenyodfat opened this issue Oct 5, 2019 · 6 comments · Fixed by #234
Closed

Adopt sendTransaction Logic From Client Library #206

orenyodfat opened this issue Oct 5, 2019 · 6 comments · Fixed by #234

Comments

@orenyodfat
Copy link
Contributor

orenyodfat commented Oct 5, 2019

this is needed for https://github.com/dOrgTech/DAOcreator

@ben-kaufman
Copy link
Contributor

I don't understand how it is related to the migration repo. Transaction replacement can be done through Metamask and adding it to an automated script like we have is problematic.

@dOrgJelli
Copy link
Contributor

dOrgJelli commented Oct 7, 2019

@ben-kaufman can you describe what you mean by "transaction replacement can be done through Metamask"? Yes Metamask currently supports "speeding up transactions", or replacing your prior one with one of higher gas cost/limit, but dApps need to support this on their end since the txHash the dApp is waiting on will change. Here's an article talking about this.

Currently the Client library supports this like so

I think the migration script should use this sendTransaction function as it (1) supports sped up & canceled transactions and (2) accurately estimates gas for transactions. Currently when using the migration script for main-net deployments, I'm not able to consistently get my transactions to confirm when using the traditional:

const block = await web3.eth.getBlock("latest");
const opts = {
  from: web3.eth.defaultAccount,
  gas: block.gasLimit - 100000
};

@orenyodfat in my opinion I would rename this issue to "Adopt sendTransaction Logic From Client Library"

@orenyodfat orenyodfat changed the title support speed-up transactions in migration script Adopt sendTransaction Logic From Client Library Oct 7, 2019
@ben-kaufman
Copy link
Contributor

@dOrgJelli I couldn't find there the exact part which supports transaction replacement. As far as I understand, the script needs to know if a replacement transaction was sent and then accordingly switch to use it instead, but I don't see such logic in the client library. Can you please point me to where it is implemented there?

@dOrgJelli
Copy link
Contributor

dOrgJelli commented Oct 7, 2019

Edit: see comment below.

I believe it is this line, a new 'transactionHash' event is sent, but I'd have to test to verify. I would right now but I'm a bit strapped for time and can't.

@dOrgJelli
Copy link
Contributor

@orenyodfat @ben-kaufman @jellegerbrandy @tspoff

I'm so sorry for misleading you all, but after running some tests it turns out this code actually fails to keep track of sped-up transactions. It isn't easy to spot this from within Alchemy because it updates its state from the subgraph, and not from the pending transaction.

Looking into how you keep track of sped up transactions now.

@dOrgJelli
Copy link
Contributor

I've posted a potential solution to this here:
daostack/arc.js#318

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 a pull request may close this issue.

3 participants