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

Move creating transaction request parameters to Transaction module #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BogdanDidenko
Copy link
Contributor

Removed code duplicates. Encapsulate creating transaction parameters in Transaction module

@qywang2012
Copy link
Contributor

@BogdanDidenko Thanks the PR, but API and admin need not require the transaction module, because we have neb-light and neb package library, they need not depend on lots of cryptos libraries.

@BogdanDidenko
Copy link
Contributor Author

BogdanDidenko commented Apr 6, 2018

It's bad solution because in all places you have the request structure

tx = {
        "from": options.from,
        "to": options.to,
        "value": utils.toString(options.value),
        "nonce": options.nonce,
        "gasPrice": utils.toString(options.gasPrice),
        "gasLimit": utils.toString(options.gasLimit),
        "contract": options.contract,
        "binary": options.binary
};

And If you change some commands or parameter type on server you should change it in all places. And this approach can have potential problems with backward compatibility and cetera

@qywang2012
Copy link
Contributor

Yeah, We may need a util or parameter js file to handle the parameters check.

@BogdanDidenko
Copy link
Contributor Author

Maybe some validation module with strategies for different types of parameters.

@qywang2012
Copy link
Contributor

Later We can add the validation module.

@BogdanDidenko
Copy link
Contributor Author

Yes. It's not top-priority task.

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