Skip to content
This repository was archived by the owner on Apr 11, 2021. It is now read-only.

Comments

[Feat] Use rawTransaction#56

Merged
annieke merged 8 commits intomasterfrom
feat/use_raw_transaction
Mar 16, 2021
Merged

[Feat] Use rawTransaction#56
annieke merged 8 commits intomasterfrom
feat/use_raw_transaction

Conversation

@karlfloersch
Copy link
Contributor

@karlfloersch karlfloersch commented Mar 10, 2021

Overview

Addresses https://github.com/ethereum-optimism/roadmap/issues/757

Make the batch submitter use rawTransaction instead of doing crazy encoding.

@karlfloersch
Copy link
Contributor Author

TODO: update package.json to use new provider once merged

@annieke annieke force-pushed the feat/use_raw_transaction branch from 52d3c19 to 08d9461 Compare March 13, 2021 00:29
@annieke annieke marked this pull request as ready for review March 13, 2021 00:42
@annieke annieke changed the title First pass using rawTransaction [Feat] Use rawTransaction Mar 13, 2021
Copy link
Contributor Author

@karlfloersch karlfloersch left a comment

Choose a reason for hiding this comment

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

Awesome! There were a few comments left around but overall this PR is already looking quite good! Didn't look into why CI is failing but I'd imagine something super simple

@annieke annieke force-pushed the feat/use_raw_transaction branch from d034e60 to 79b016a Compare March 15, 2021 19:23
Copy link
Contributor Author

@karlfloersch karlfloersch left a comment

Choose a reason for hiding this comment

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

Left one nit but this looks good!

Oh I also can't approve this PR cuz I created it whoops

}
}

export const formatNumber = (num: any): number => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be num: string?

Is there a function which does this in core-utils? If not I think it's fine to include here but eventually probably a good idea to move it there

Copy link
Contributor

Choose a reason for hiding this comment

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

i copied this from ethers formatter, which i think is supposed to keep the input general since it may begin as a number too

https://github.com/ethers-io/ethers.js/blob/6c43e20e7a68f3f5a141c74527ec63d9fe8458be/packages/providers/src.ts/formatter.ts#L165-L170

i'll move it into core-utils in a followup PR :)

Copy link
Contributor

@annieke annieke left a comment

Choose a reason for hiding this comment

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

lgtm 🥇 💯

@annieke annieke merged commit d4de023 into master Mar 16, 2021
@annieke annieke deleted the feat/use_raw_transaction branch March 16, 2021 00:03
@karlfloersch karlfloersch restored the feat/use_raw_transaction branch March 22, 2021 18:06
karlfloersch added a commit that referenced this pull request Mar 22, 2021
karlfloersch added a commit that referenced this pull request Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants