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

(Feature) Enable proxy for both strategies #1024

Merged
merged 16 commits into from
Jul 13, 2018
Merged

(Feature) Enable proxy for both strategies #1024

merged 16 commits into from
Jul 13, 2018

Conversation

vbaranov
Copy link
Collaborator

@vbaranov vbaranov commented Jul 6, 2018

Closes #1022
Closes #957
Closes #1025

The last PR to enable Proxy contracts.

  • Proxies registry smart-contract was deployed for Ropsten
  • Exec-id is eliminated. Instead, proxy address is used.
  • ./exec-id query param => ./addr query param
  • registryExec.exec(...) => ...proxy.exec(...)
  • all getters ...idx => ...proxy

@dennis00010011b
Copy link

dennis00010011b commented Jul 8, 2018

@vbaranov
MInted-Manage page: incorrect maxCap for whitelist
screen shot 2018-07-08 at 06 28 17

@dennis00010011b
Copy link

dennis00010011b commented Jul 8, 2018

@vbaranov
Dutch-Manage page: link to crowdsale page is broken
screen shot 2018-07-08 at 07 38 40

@dennis00010011b
Copy link

dennis00010011b commented Jul 8, 2018

@vbaranov
Dutch with whitelist-Contribution page: error when whitelisted investor contributes
Proxy addresses:
0x8bc98fe870d28090a2e309d3ef6f55dd1c6ff2f8 ,Ropsten
0xa81262b49c8178ff95aa9441687dbdb46f8cc500,Ropsten
dutch_proxy.log

screen shot 2018-07-08 at 08 01 42

@vbaranov
Copy link
Collaborator Author

vbaranov commented Jul 9, 2018

@dennis00010011b

MInted-Manage page: incorrect maxCap for whitelist

Fixed

Dutch-Manage page: link to crowdsale page is broken
Dutch with whitelist-Contribution page: error when whitelisted investor contributes

I cannot repeat both of them. Could you provide steps to reproduce?

@dennis00010011b
Copy link

dennis00010011b commented Jul 9, 2018

@vbaranov

Dutch-Manage page: link to crowdsale page is broken

Steps for reproduce:

  1. Create dutch crowdsale with any parameters
  2. Open manage page as an owner
  3. Click link Crowdsale page that located below button Finalize
    Expected result: crowdsale page should contains actual info about crowdsale
    Actual result: link leads to empty page
    Proxy:0xde9b82a5860e6e5bea78d1ae0d6cbaa485489878, Ropsten

@dennis00010011b
Copy link

dennis00010011b commented Jul 9, 2018

@vbaranov

Dutch with whitelist-Contribution page: error when whitelisted investor contributes

Still reproducible. Steps:

  1. Create dutch crowdsale with one whitelisted address
  2. Wait until crowdsale starts
  3. Set whitelisted account in metamask
  4. Buy allowed amount: fill out field contribute with value more than minCap and click button contribute
    Proxy:0xde9b82a5860e6e5bea78d1ae0d6cbaa485489878 , Ropsten

screen shot 2018-07-09 at 06 03 26

@vbaranov
Copy link
Collaborator Author

vbaranov commented Jul 9, 2018

@dennis00010011b

Dutch-Manage page: link to crowdsale page is broken

Steps for reproduce:

Create dutch crowdsale with any parameters
Open manage page as an owner
Click link Crowdsale page that located below button Finalize
Expected result: crowdsale page should contains actual info about crowdsale
Actual result: link leads to empty page
Proxy:0xde9b82a5860e6e5bea78d1ae0d6cbaa485489878, Ropsten

ah, makes sense now, thanks, fixed.

Dutch with whitelist-Contribution page: error when whitelisted investor contributes

Could you post log from Chrome developer console? I still cannot reproduce the issue

@dennis00010011b
Copy link

@vbaranov
dutch-contr.log

@vbaranov
Copy link
Collaborator Author

vbaranov commented Jul 9, 2018

@dennis00010011b I've added handling of gas estimation for buy function. Please, let me know the results of new test.

@dennis00010011b
Copy link

dennis00010011b commented Jul 9, 2018

@vbaranov
Dutch: Incorrect amount passes to Metamask. Investor can't buy, any contribution's transaction is failed(but owner still can modify whitelist).

screen shot 2018-07-09 at 07 18 31

Failed transactions
https://ropsten.etherscan.io/tx/0x717c30172cd2affafd4aa6f844f9dac9d6a14dacb0bd338a58377e8990e16d0b

https://ropsten.etherscan.io/tx/0xb5aa7bdd0f8185b0024b20ab0d9fb5538339cc4fb4be2ccb07956ba065c3e9e6

@vbaranov
Copy link
Collaborator Author

vbaranov commented Jul 9, 2018

@dennis00010011b do you have the same problem in 2.0 branch? if yes, please open a separate issue.

I repeated all steps:

Create dutch crowdsale with one whitelisted address
Wait until crowdsale starts
Set whitelisted account in metamask
Buy allowed amount: fill out field contribute with value more than minCap and click button contribute

and error doesn't exist for me. If error still exists for you please record a video.

@dennis00010011b
Copy link

@vbaranov

Dutch with whitelist-Contribution page: error when whitelisted investor contributes

This issue was fixed with last commit . Instead I can't buy from Dutch crowdsale #1024 (comment)

@dennis00010011b
Copy link

dennis00010011b commented Jul 9, 2018

@vbaranov
https://drive.google.com/open?id=1eMDZSXSugpLLoSRa5IpvMZZX3Lefza3u
dutch-meta.log
Proxy address: 0x70989912ccdd685b13a27c4c567438149efeb77d , ropsten

Crowdsale parameters (one more whitelist account was added later )
{
"name": "scenarioDutchRopsten.json",
"ticker": "test",
"decimals": 10,
"totalSupply":100,
"walletAddress":"0x56B2e3C3cFf7f3921Dc2e0F8B8e20d1eEc29216b",
"gasprice":100,
"tiers":[
{
"isWhitelisted":true,
"startDate": "360000",
"startTime": "360000",
"endDate":"72000000",
"endTime":"72000000",
"minCap":11,
"minRate":1000,
"maxRate":10000,
"supply": 100,
"whitelist":[{
"address": "0xDb0E42Ee8fDaAd8A7f295f447F71A1480c98a373",
"min": 50,
"max": 150
} ]
}]
}

Copy link

@dennis00010011b dennis00010011b left a comment

Choose a reason for hiding this comment

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

Tested, no issues were found

@vbaranov
Copy link
Collaborator Author

vbaranov commented Jul 10, 2018

@dennis00010011b I have fixed #1025 also. Please test it again.

@vbaranov vbaranov changed the title (2.0) Enable proxy for both strategies (Fix) Enable proxy for both strategies Jul 10, 2018
@vbaranov vbaranov changed the title (Fix) Enable proxy for both strategies (Feature) Enable proxy for both strategies Jul 10, 2018
@dennis00010011b
Copy link

@vbaranov
Tested it . No issues were found

@vbaranov
Copy link
Collaborator Author

@fernandomg looking forward to your review.

Copy link
Contributor

@fernandomg fernandomg left a comment

Choose a reason for hiding this comment

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

PR is working fine. All the suggested changes are in order to preserve consistency in the code.

try {
estimatedGas = await method.estimateGas(opts)
} catch (e) {
console.log(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

logger.error instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

let tokensSold = await getTokensSold(...params).call()
console.log('tokensSold:', tokensSold)
Copy link
Contributor

Choose a reason for hiding this comment

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

logger.log (I'm the logger police 👮‍♂️)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

crowdsaleStatus.time_remaining = crowdsaleStatus.time_remaining || crowdsaleStatus[4]
const tokens_remaining = crowdsaleStatus.tokens_remaining || crowdsaleStatus[5]
const _isCrowdsaleFull = await isCrowdsaleFull(...params).call()
const max_sellable = _isCrowdsaleFull.max_sellable || _isCrowdsaleFull[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why in some cases you use the array lookup for the value as an alternative, and in other cases just go with the object key lookup like in?:

https://github.com/poanetwork/token-wizard/pull/1024/files#diff-5c25cfce1eba8feb7668398c93446a0bR118

const wei_raised = crowdsaleInfo.wei_raised

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I use the array lookup because Proxy getters return only arrays. In the line, that you provided, array lookup wasn't made because it was made earlier in this line:

crowdsaleInfo.wei_raised = crowdsaleInfo[0]

return [
() => {
logger.log('###trackProxy:###')
console.log('contractStore:', contractStore)
Copy link
Contributor

Choose a reason for hiding this comment

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

logger.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

toJS(contractStore.ProxiesRegistry.abi),
contractStore.ProxiesRegistry.addr
)
console.log('contractStore[crowdsaleStore.proxyName].addr:', contractStore[crowdsaleStore.proxyName].addr)
Copy link
Contributor

Choose a reason for hiding this comment

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

logger.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@@ -344,6 +344,46 @@ async function getAllApplicationsInstances() {
return Promise.all(whenCrowdsales).then(crowdsales => crowdsales.filter(crowdsale => crowdsale !== null))
}

async function getOwnerApplicationsInstancesForProxy() {
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest you a refactor for this method?

As suggested in this SO Answer, the way to go is with a map. I used a reduce to enforce the order, having the ability to list from newer to older the crowdsales created.

async function getOwnerApplicationsInstancesForProxy() {
  const { web3 } = web3Store
  const proxiesRegistryContract = await attachToSpecificCrowdsaleContract('ProxiesRegistry')
  const accounts = await web3.eth.getAccounts()
  const execIDs = await proxiesRegistryContract.methods.getCrowdsalesForUser(accounts[0]).call()

  const { abi } = contractStore.MintedCappedProxy
  const mintedCapped = process.env[`${REACT_PREFIX}MINTED_CAPPED_APP_NAME`].toLowerCase()
  const dutchAuction = process.env[`${REACT_PREFIX}DUTCH_APP_NAME`].toLowerCase()

  return Promise.all(
    execIDs.reduce((promise, execID) => {
      return promise.then(async () => {
        try {
          const contractInstance = await attachToContract(abi, execID)
          const contractAppName = await contractInstance.methods.app_name.call()
          const appName = removeTrailingNUL(web3.utils.toAscii(contractAppName)).toLowerCase()

          if (appName.includes(mintedCapped) || appName.includes(dutchAuction)) {
            return { appName, execID }
          }
        } catch (error) {
          logger.error(error)
        }
      })
    }, Promise.resolve())
  )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fernandomg Unfortunately, such refactor won't work because return doesn't return an iterable object:
screen shot 2018-07-13 at 13 52 47

I've implemented all useful and working parts from your suggestion. Thanks for sharing. Let's return to the final refactor of getOwnerApplicationsInstancesForProxy function in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants