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

Fix invite script by moving to contractkit #1645

Merged
merged 6 commits into from
Nov 10, 2019
Merged

Conversation

cmcewen
Copy link
Contributor

@cmcewen cmcewen commented Nov 9, 2019

Description

Invite script was broken so just decided to move to celotool + contractkit

Tested

Ran locally and got a text with a valid invite

Backwards compatibility

Shouldn't break anything

packages/celotool/src/cmds/account/invite.ts Show resolved Hide resolved
packages/celotool/src/cmds/account/invite.ts Outdated Show resolved Hide resolved
console.log(`Using account: ${account}`)
kit.defaultAccount = account

// TODO(asa): This number was made up
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels dangerous but I guess it's been working for us

const verificationFee = new BigNumber(
await attestations.attestationRequestFees(stableToken.address)
)
const goldAmount = verificationGasAmount.times(gasPrice).toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to send gold anymore at all, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if anyone uses this + pays gas in gold so I just added a comment to remove it - just hesitant to do it when not everyone will have a chance to look at this

@codecov
Copy link

codecov bot commented Nov 9, 2019

Codecov Report

Merging #1645 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #1645    +/-   ##
========================================
  Coverage   74.23%   74.23%            
========================================
  Files         287      287            
  Lines        7704     7704            
  Branches      667      960   +293     
========================================
  Hits         5719     5719            
  Misses       1871     1871            
  Partials      114      114
Flag Coverage Δ
#mobile 74.23% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 930bf31...59ff713. Read the comment docs.

)
await Promise.all([
// TODO: remove if no one is paying for gas with gold
goldToken.transfer(temporaryAddress, goldAmount).sendAndWaitForReceipt(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, i think your comment is right. The invite doesn't actually need to send gold.

kit.defaultAccount = account

// TODO(asa): This number was made up
const attestationGasAmount = new BigNumber(10000000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure we can cut this and the gold related parts too

@cmcewen cmcewen added the automerge Have PR merge automatically when checks pass label Nov 10, 2019
@celo-ci-bot-user celo-ci-bot-user merged commit 1923f86 into master Nov 10, 2019
@celo-ci-bot-user celo-ci-bot-user deleted the cmcewen/new-invite branch November 10, 2019 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants