-
Notifications
You must be signed in to change notification settings - Fork 10
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
Integration with mosaic.js npm - 0.10.0-beta.2 #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…s_integration # Conflicts: # lib/helper/GnosisSafe.js # package.json # test/integration/DirectTransfer.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (we can merge), left some inline comments.
@@ -22,18 +21,32 @@ class OpenSTAbiBinProvider extends AbiBinProvider { | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why OpenSTAbiBinProvider is inheriting from AbiBinProvider of mosaic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenST is on top of mosaic. Inheriting AbiBinProvider of mosaic helps end users to retrieve abi/bin of mosaic with a single OpenST AbiBinProvider object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But should they need to? Why?
binFolderPath = binFolderPath || DEFAULT_BIN_FOLDER_PATH; | ||
super(abiFolderPath, binFolderPath); | ||
constructor(abiFolderPath, binFolderPath) { | ||
const abiDirectoryPath = abiFolderPath || DEFAULT_ABI_FOLDER_PATH; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEFAULT_ABI_FOLDER_PATH and DEFAULT_BIN_FOLDER_PATH are guarded (NOT_FOR_WEB__BEGIN). Should we check that if they are undefined and abiFolderPath, binFolderPath are not passed and raise error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. However I am still not sure how can DEFAULT_ABI_FOLDER_PATH, DEFAULT_BIN_FOLDER_PATH can be undefined.
Having said that once openst-contracts npm package are integrated, we will not need path dependencies .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhayks1 can you please expand on that? I cannot follow.
Integration of mosaic.js npm 0.10.0-beta.2
Fixes #76