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

Updated OSTPrime contract and tests. #533

Merged
merged 18 commits into from
Dec 12, 2018
Merged

Conversation

deepesh-kn
Copy link
Contributor

fixes: #417

  • This PR includes the unit test of all the functions of OST prime.
  • The Claim event is renamed to TokenUnwrapped.
  • The Redeem event is renamed to TokenWrapped.
    - uint256 _totalSupply and address _utilityToken params are removed from both the events as it is not needed.
  • Added a check so thatinitialize function can be called only once.
  • claim function is renamed to unwrap.
  • redeem function is renamed to wrap.
  • Both wrap and unwrap functions are modified.
  • Unit tests are covered for all the functions of OST prime.
    Please note: The unit tests for UtilityToken and EIP20Token is not covered in this. They are the separate tests.

test/gateway/ost_prime/wrap.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/wrap.js Outdated Show resolved Hide resolved
tools/runGanacheCli.sh Outdated Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Show resolved Hide resolved
test/gateway/ost_prime/constructor.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/initialize.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/constructor.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/unwrap.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/unwrap.js Outdated Show resolved Hide resolved
Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

Phew, big one. Nice work! 💯
I added my comments.

contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Show resolved Hide resolved
test/gateway/ost_prime/wrap.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/wrap.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/wrap.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/wrap.js Outdated Show resolved Hide resolved
tools/runGanacheCli.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@0xsarvesh 0xsarvesh left a comment

Choose a reason for hiding this comment

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

Nice 🙌 🚀
Some feedback inline.

contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
test/gateway/ost_prime/unwrap.js Outdated Show resolved Hide resolved

let brandedTokenAddress, ostPrime, callerAddress, amount;

async function initialize(){
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used in both wrap and unwrap, can be extracted out.
Also the config of OSTPrime.

contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@0xsarvesh 0xsarvesh left a comment

Choose a reason for hiding this comment

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

LGTM 🎢 🚋 💯

Just one comment:
is there still value in writing unit test scenario where it tests wrapping of max possible amount i.e balance - gas ?

Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

✅ 🎊

@deepesh-kn what is your confidence level now?

contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
test/gateway/ost_prime/unwrap.js Outdated Show resolved Hide resolved
test/test_lib/utils.js Show resolved Hide resolved
@deepesh-kn
Copy link
Contributor Author

@deepesh-kn what is your confidence level now?

My confidence level is very good.
What do you think as a reviewer?

@deepesh-kn
Copy link
Contributor Author

Just one comment:
is there still value in writing unit test scenario where it tests wrapping of max possible amount i.e balance - gas ?

This is the same as one of the existing test.

@gulshanvasnani
Copy link
Contributor

LGTM
Great Work 👍

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.

Unit test all the functions of OSTPrime.
5 participants