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

Improve web3 options handling and single loadContracts #245

Merged
merged 4 commits into from
Feb 7, 2021

Conversation

hellwolf
Copy link
Contributor

@hellwolf hellwolf commented Feb 6, 2021

No description provided.

@hellwolf hellwolf marked this pull request as ready for review February 6, 2021 22:01
@hellwolf hellwolf requested a review from pi0neerpat February 6, 2021 22:01
@hellwolf hellwolf marked this pull request as draft February 6, 2021 22:01
@github-actions
Copy link

github-actions bot commented Feb 6, 2021

📦 PR Packages

Install this PR:

yarn add @superfluid-finance/js-sdk@PR245 --registry https://npm.pkg.github.com/
:octocat: Click to learn how to use Github packages

To use the Github package registry, create a token with "read:packages" permission. See Creating a personal access token for help.

Next add these lines to your ~/.npmrc file, replacing TOKEN with your personal access token. See Installing a package from Github if you get stuck.

@superfluid-finance:registry=https://npm.pkg.github.com
//npm.pkg.github.com/:_authToken=TOKEN

@hellwolf hellwolf marked this pull request as ready for review February 6, 2021 23:20
Copy link
Contributor

@pi0neerpat pi0neerpat left a comment

Choose a reason for hiding this comment

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

looks good, just a few comments for my own understanding

Comment on lines +84 to +91
if (from) {
console.log("Set default from address to", from);
} else {
const accounts = await web3.eth.getAccounts();
from = accounts[0];
console.log(
"Set default from address to the first account",
from
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I didn't want to do this, is b/c I thought maybe a test will want to change from after the contract is loaded. If thats not the case then this is ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

each transaction can always use the non-default from address.

this was added because it seems otherwise the deployment scripts didn't work...

});
} catch (e) {
throw Error(
`could not load non-truffle environment contracts. ${e}`
);
}
} else {
} else if (isTruffle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add back isTruffle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to have an explicit error thrown on unknown mode.

this.isTruffle = isTruffle;
this.ethers = ethers;
Copy link
Contributor

Choose a reason for hiding this comment

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

no longer can deploy test environment with ethers?

*
* Usage: npx truffle exec scripts/reset-deployment.js : {VERSION}
*/
module.exports = async function(callback, argv) {
try {
global.web3 = web3;
Copy link
Contributor

Choose a reason for hiding this comment

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

ah maybe this is why ethers test were failing

const config = Superfluid.getConfig(chainId);
console.log("reset: ", reset);
console.log("chain ID: ", chainId);
eval(`(${detectTruffleAndConfigure.toString()})(options)`);
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 scary

@hellwolf hellwolf merged commit 2ab0107 into dev Feb 7, 2021
@hellwolf hellwolf deleted the improve_web3_arguments_and_load_contracts branch February 7, 2021 19:26
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.

2 participants