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

Get final converted amount via abi interface #38

Merged
merged 1 commit into from
Jun 12, 2020

Conversation

andy-hook
Copy link

@andy-hook andy-hook commented Jun 11, 2020

Address #37 (comment)

lib/web3-contracts.js Outdated Show resolved Hide resolved
@andy-hook andy-hook requested a review from sohkai June 11, 2020 15:47
try {
const transactionReceipt = await ethersProvider.getTransactionReceipt(
hash
)

return transactionReceipt
transactionReceipt.logs.forEach(log => {
const parsedLog = abiInterface.parseLog(log)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to make sure:

  • What happens if that specific log can't be parsed? Is an error thrown or is parsedLog just null?
  • Here, since we control the previous transaction, we can just assume that the first log is going to be the Transfer event (I believe it is, anyway—there should only be two logs in these claim transactions).

Copy link

@0xGabi 0xGabi Jun 11, 2020

Choose a reason for hiding this comment

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

To answer the first question I look into Ethers code and the library return null if the log can't be parsed: https://github.com/ethers-io/ethers.js/blob/master/utils/interface.js#L388

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool—can we double check what happens with the v5 ABI encoder?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for looking @0xGabi , I'll log the encoder as an item to check.

Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

Some small comments, but LGTM!

Also asked @0xGabi to take a quick look, since he's had the most experience with ethers.Interface :).

@andy-hook andy-hook force-pushed the signing-stepper__logs-via-abi branch from 62bb274 to a4780c4 Compare June 12, 2020 08:55
@andy-hook andy-hook merged commit 0ec4dc8 into master Jun 12, 2020
@andy-hook andy-hook deleted the signing-stepper__logs-via-abi branch June 12, 2020 09:16
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.

3 participants