Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

swap: unify variable types, pt. 2 #2082

Merged
merged 10 commits into from
Jan 30, 2020
Merged

Conversation

mortelli
Copy link
Contributor

@mortelli mortelli commented Jan 27, 2020

This PR is part 2 of implementing issue #1581, which aims at unifying variable types between the SWAP smart contracts and the related code on the go side.

The strategy is to work with the Uint256 type right up until calling the contract bindings. Only then should the code work with the big.Int type.

The following functions have had their signature modified to work with the Uint256 type:

  • CashChequeBeneficiaryStart
  • estimatePayout
  • newSignedTestCheque
  • testDeployWithPrivateKey
  • testDeploy
  • newSwapTester

Other functions which call these have had their bodies updated as a result.

Copy link
Member

@ralph-pichler ralph-pichler left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@holisticode holisticode left a comment

Choose a reason for hiding this comment

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

Approved, but I recall you said that we would change the balance to be a Uint256 with the part 2 of the unify variable types part. Will there be a part 3 then?

Also, it still looks odd to my eyes that to set a Uint256 value to zero you are calling FromUint64, but as the downsides of this are very negligible, I don't want to make a big fuss about it :)

@mortelli
Copy link
Contributor Author

Approved, but I recall you said that we would change the balance to be a Uint256 with the part 2 of the unify variable types part.

thanks. balances have sign, therefore they cannot be of the Uint256 type.

i have added this matter for discussion during the next incentives call.

Will there be a part 3 then?

when the last PR is submitted for this, i will make it clear that there won't be any more parts.

Also, it still looks odd to my eyes that to set a Uint256 value to zero you are calling FromUint64, but as the downsides of this are very negligible, I don't want to make a big fuss about it :)

i think this comment i made in the previous PR (part 1) somewhat addresses this point, but we didn't really continue the thread.

what do you think?

@mortelli mortelli merged commit a091c6b into master Jan 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants