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

placeSellTokenOrder & placeBuyTokenOrder #42

Merged
merged 6 commits into from
May 7, 2019
Merged

placeSellTokenOrder & placeBuyTokenOrder #42

merged 6 commits into from
May 7, 2019

Conversation

szerintedmi
Copy link
Member

No description provided.

src/Exchange.ts Outdated Show resolved Hide resolved
role,
contractListStub.map(contractStub => new DeployedContract(contractStub))
);
deployedEnvironment.addRole(role, contractListStub.map(contractStub => new DeployedContract(contractStub)));
Copy link
Contributor

Choose a reason for hiding this comment

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

is this some autoformatting? did the settings change?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep i've noticed. it's happened before too. I suspect that @rszaloki doesn't have prettier format on save set or something else different in our editor. We should make sure we sync our setting

src/Exchange.ts Outdated
@@ -202,6 +207,53 @@ export class Exchange extends AbstractContract {
return orders;
}

public placeSellTokenOrder(price: number, amount: number): Transaction {
Copy link
Contributor

Choose a reason for hiding this comment

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

as always, I suggest using integer bignums in lowest denomination (BN) consistently throughout the entire library, which would eliminate the need for the multiplication and integer checks below.

Copy link
Member Author

@szerintedmi szerintedmi May 6, 2019

Choose a reason for hiding this comment

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

  1. We use BigNumber and not BN - which is already an issue. But web3.js is keep changing what do they use and how. There is an ongoing discussion about it. Shall we wait for that to settle before we refactor?

  2. As always I'm for dev UX to augmint-js consumers.
    I.e. in case value can be represented with number then I would prefer

     exchange.placeSellTokenOrder(1.01, 100.12)

    over

    const decimalsDiv  = await exchange.token.decimalsDiv;
    exchange.placeSellTokenOrder(1.01, decimalsDiv.mul(100.12) )

Not a huge difference just seems easier.

Copy link
Contributor

@phraktle phraktle May 6, 2019

Choose a reason for hiding this comment

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

Linked in the proposal, ECMAScript BigInt seems to be best approach (being standardized in the language), with consideration given to more specialized sub-types (eg. wei). Since the point would be consistency, I'm happy to defer this to a later effort in a separate PR.

However, in the meantime let's at least get rid of the copy-pasted integrity checks and replace them with something that performs both unit conversions and integrity checks at the same time.

So instead of

const amountToSend: number = amount * DECIMALS_DIV;
if (Math.round(amountToSend) !== amountToSend) {
    throw new InvalidTokenAmountError(
        ` placeSellTokenOrder error: provided price of ${amountToSend} has more decimals than allowed by AugmintToken decimals of ${DECIMALS}`
    );
}

I would rather see something along the lines of:

const rawAmountToSend = AugmintToken.toRawAmount(amount);

(where the term raw would always mean numbers certain to be integers, in the lowest unit)

Copy link
Member Author

@szerintedmi szerintedmi May 6, 2019

Choose a reason for hiding this comment

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

sum of chat: we use BN class from bn.js (see ddb1121)

src/Augmint.ts Outdated Show resolved Hide resolved
@szerintedmi szerintedmi requested a review from phraktle May 6, 2019 19:47
@phraktle phraktle merged commit 17433d6 into staging May 7, 2019
@phraktle phraktle deleted the placeorder branch May 7, 2019 03:48
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