Skip to content
This repository has been archived by the owner on Feb 6, 2019. It is now read-only.

Airdrop Contract #49

Merged
merged 13 commits into from
Feb 17, 2018
177 changes: 177 additions & 0 deletions contracts/Airdrop.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
/* solhint-disable-next-line compiler-fixed */
pragma solidity ^0.4.17;

// Copyright 2018 OpenST Ltd.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
// ----------------------------------------------------------------------------
// Utility chain: Airdrop
//
// http://www.simpletoken.org/
//
// ----------------------------------------------------------------------------

import "./Workers.sol";
import "./Pricer.sol";


contract Airdrop is Pricer, Workers {
Copy link
Contributor

Choose a reason for hiding this comment

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

Airdrop is not Workers


/*
* Events
*/
/// Event for AirdropPayment complete
event AirdropPayment(
Copy link
Contributor

Choose a reason for hiding this comment

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

up to three arguments can be indexed and it makes them (expanded and) searchable in the logs; also for formatting on events we've previously filled the line (different from function signatures)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indexing is cool. Does indexing cost extra gas or it's neglegible?
Can we do indexing on uint256 also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also for formatting on events we've previously filled the line (different from function signatures)
Didn't understand this point. Please explain with an example?

Copy link
Contributor

@jasonklein jasonklein Feb 15, 2018

Choose a reason for hiding this comment

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

@benjaminbollen: for consistency/ease, we started declaring events similar to functions,

event EventName(
arg,
arg,
arg)

This way it not subject to personal preference in terms of how many is too many arguments to list on the line. We can go back to the other way, if preferred, but that will mean changes in other rule for max number of arguments per line (so that how it looks in an individual dev's editor is not driving the decision).

Copy link
Contributor

Choose a reason for hiding this comment

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

the event AirdropPayment is missing the quote currency; worth adding if we have the price point?

address _beneficiary,
uint256 tokenAmount,
address _commissionBeneficiary,
uint256 commissionTokenAmount,
uint256 _actualPricePoint,
address _spender,
uint256 _airdropAmount
);

/*
* Storage
*/
Workers public workers;
address public airdropBudgetHolder;

/*
* Constructor
*/
/// @dev Takes _brandedToken, _baseCurrency, _workers, _airdropBudgetHolder;
/// constructor;
/// public method;
/// @param _brandedToken Branded Token
/// @param _baseCurrency Base Currency
/// @param _workers Workers contract address
/// @param _airdropBudgetHolder Airdrop Budget Holder Address
function Airdrop(
address _brandedToken,
bytes3 _baseCurrency,
Workers _workers,
address _airdropBudgetHolder)
public
Pricer(_brandedToken, _baseCurrency)
OpsManaged()
{
require(_workers != address(0));
require(airdropBudgetHolder != address(0));

workers = _workers;
airdropBudgetHolder = _airdropBudgetHolder;
}

/*
* External functions
*/
/// clean up or revoke airdrop contract
function remove()
Copy link
Contributor

Choose a reason for hiding this comment

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

should the remove function not be implemented in Pricer.sol, and can be inherited here without change.

external
onlyAdminOrOps
{
selfdestruct(msg.sender);
Copy link
Contributor

Choose a reason for hiding this comment

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

an event before self-destruct is useful

Copy link
Contributor Author

@abhayks1 abhayks1 Feb 15, 2018

Choose a reason for hiding this comment

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

adding event "Removed"

}

/*
* External functions
*/
/// performPay matches the behaviour of Pricer:pay with extra functionality of airdrop evaluation
Copy link
Contributor

Choose a reason for hiding this comment

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

performPay is not a suitable name;
least preferred, performAirPay on Airdrop; and performPay on Pricer; but we dont prefix with perform for core external function calls

so airdropPay on Airdrop; and pay on pricer;
or payAirdrop on Airdrop; and pay on pricer;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

payAirdrop on Airdrop; and pay on pricer;
Sounds good.

/// @param _beneficiary beneficiary
/// @param _transferAmount transferAmount
/// @param _commissionBeneficiary commissionBeneficiary
/// @param _commissionAmount commissionAmount
/// @param _currency currency
/// @param _intendedPricePoint intendedPricePoint
/// @param _spender spender
/// @param _airdropAmount airdropAmount
/// @return uint256 totalPaid
function performPay(
address _beneficiary,
uint256 _transferAmount,
address _commissionBeneficiary,
uint256 _commissionAmount,
bytes3 _currency,
uint256 _intendedPricePoint,
address _spender,
uint256 _airdropAmount)
public
returns (
uint256 /* totalPaid */)
{
require(workers.isWorker(msg.sender));
require(_spender != address(0));

require(isValidBeneficiaryData(_beneficiary, _transferAmount, _commissionBeneficiary, _commissionAmount));

uint256 tokenAmount = _transferAmount;
uint256 commissionTokenAmount = _commissionAmount;
uint256 pricePoint = _intendedPricePoint;

// check Margin And Calculate BTAmount
if (_currency != "") {
(pricePoint, tokenAmount, commissionTokenAmount) = validateMarginAndCalculateBTAmount(_currency,
_intendedPricePoint, _transferAmount, _commissionAmount);
}

require(performAirdropTransferToSpender(_spender, _airdropAmount,
tokenAmount, commissionTokenAmount));
require(performTransfer(_spender, _beneficiary, tokenAmount,
_commissionBeneficiary, commissionTokenAmount));

/// Emit AirdropPayment Event
AirdropPayment(_beneficiary, tokenAmount, _commissionBeneficiary,
commissionTokenAmount, pricePoint, _spender, _airdropAmount);

return ((tokenAmount + commissionTokenAmount));
}

/*
* Private functions
*/
/// @dev Takes _spender, _airdropAmount, _tokenAmount, _commissionTokenAmount;
/// Calculate airdropUsed to transfer
/// Perform perform Airdrop Transfer To Spender
/// internal method;
/// @param _spender spenderUser
/// @param _airdropAmount airdropAmount
/// @param _tokenAmount tokenAmount
/// @param _commissionTokenAmount commissionTokenAmount
/// @return uint256 airdropUsed
function performAirdropTransferToSpender(
address _spender,
uint256 _airdropAmount,
uint256 _tokenAmount,
uint256 _commissionTokenAmount)
private
returns (
bool /* boolean value */)
{
// @Ben Do we need this? Can't we transfer all _airdropAmount to _spender?
uint256 airdropUsed = _airdropAmount;
uint256 totalPaid = (_tokenAmount + _commissionTokenAmount);
if (_airdropAmount > totalPaid) {
airdropUsed = _airdropAmount - totalPaid;
}

// Prefund the user from the airdrop budget holder
if (airdropUsed > 0) {
require(EIP20Interface(brandedToken()).transferFrom(airdropBudgetHolder, _spender, airdropUsed));
}

return true;
}

}
135 changes: 106 additions & 29 deletions contracts/Pricer.sol
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* solhint-disable-next-line compiler-fixed */
Copy link
Contributor

Choose a reason for hiding this comment

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

on L34: Pricer is not PricerInterface; the dependency direction is reversed; pricer calls on contracts, for now other contracts dont call on pricer; neither is there anything general about pricer worthy of generalising in an interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we concluded we will keep PricerInterface.sol

pragma solidity ^0.4.17;

// Copyright 2018 OpenST Ltd.
Expand Down Expand Up @@ -60,8 +61,8 @@ contract Pricer is OpsManaged, PricerInterface {
/// @dev Takes _brandedToken, _baseCurrency;
/// constructor;
/// public method;
/// @param _brandedToken _brandedToken
/// @param _baseCurrency _baseCurrency
/// @param _brandedToken Branded Token
/// @param _baseCurrency Base Currency
function Pricer(
address _brandedToken,
bytes3 _baseCurrency)
Expand Down Expand Up @@ -251,7 +252,7 @@ contract Pricer is OpsManaged, PricerInterface {
/// @param _commissionAmount commissionAmount
/// @param _currency currency
/// @param _intendedPricePoint _intendedPricePoint
/// @return bool isSuccess
/// @return uint256 total paid
function pay(
address _beneficiary,
uint256 _transferAmount,
Expand All @@ -260,36 +261,28 @@ contract Pricer is OpsManaged, PricerInterface {
bytes3 _currency,
uint256 _intendedPricePoint)
public
returns (bool /* success */)
returns (uint256 /* total paid */)
{
require(_beneficiary != address(0));
require(_transferAmount != 0);

if (_commissionAmount > 0) {
require(_commissionBeneficiary != address(0));
}
require(isValidBeneficiaryData(_beneficiary, _transferAmount,
_commissionBeneficiary, _commissionAmount));

uint256 tokenAmount = _transferAmount;
uint256 commissionTokenAmount = _commissionAmount;
uint256 pricePoint = _intendedPricePoint;
if (_currency != 0) {
pricePoint = getPricePoint(_currency);
require(pricePoint > 0);
require(isPricePointInRange(_intendedPricePoint, pricePoint, pricerAcceptedMargins[_currency]));
(tokenAmount, commissionTokenAmount) = getBTAmountFromCurrencyValue(pricePoint,
_transferAmount, _commissionAmount);
}

require(EIP20Interface(pricerBrandedToken).transferFrom(msg.sender, _beneficiary, tokenAmount));
if (_commissionBeneficiary != address(0)) {
require(EIP20Interface(pricerBrandedToken).transferFrom(msg.sender,
_commissionBeneficiary, commissionTokenAmount));

// check Margin And Calculate BTAmount
if (_currency != "") {
(pricePoint, tokenAmount, commissionTokenAmount) = validateMarginAndCalculateBTAmount(_currency,
_intendedPricePoint, _transferAmount, _commissionAmount);
}


require(performTransfer(msg.sender, _beneficiary, tokenAmount,
_commissionBeneficiary, commissionTokenAmount));

//Trigger Event for PaymentComplete
Payment(_beneficiary, _transferAmount, _commissionBeneficiary,
_commissionAmount, _currency, _intendedPricePoint, pricePoint);
return true;
_commissionAmount, _currency, _intendedPricePoint, pricePoint);
return (tokenAmount + commissionTokenAmount);
}

/// @dev Takes _currency;
Copy link
Contributor

Choose a reason for hiding this comment

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

getPricePoint needs to be a public view function; this produces a warning because Price Oracle can emit an event; but the conclusion is then to remove the event emission from getPrice in Price Oracle; this should also be be specified in the Price Oracle interface

again, the interaction layer can determine that there was no valid price point based on the return value, and an event on the get function is not needed

Expand All @@ -309,7 +302,7 @@ contract Pricer is OpsManaged, PricerInterface {

/// @dev Takes _intendedPricePoint, _currentPricePoint, _acceptedMargin;
/// checks if the current price point is in the acceptable range of intendedPricePoint;
/// private method;
/// internal method;
/// @param _intendedPricePoint intendedPricePoint
/// @param _currentPricePoint currentPricePoint
/// @param _acceptedMargin acceptedMargin
Expand All @@ -318,7 +311,7 @@ contract Pricer is OpsManaged, PricerInterface {
uint256 _intendedPricePoint,
uint256 _currentPricePoint,
uint256 _acceptedMargin)
private
internal
pure
returns (bool /*isValid*/)
{
Expand All @@ -334,7 +327,7 @@ contract Pricer is OpsManaged, PricerInterface {

/// @dev Takes _pricePoint, _transferAmount, _commissionAmount;
/// calculates the number of branded token equivalant to the currency amount;
/// private method;
/// internal method;
/// @param _pricePoint pricePoint
/// @param _transferAmount transferAmount
/// @param _commissionAmount commissionAmount
Expand All @@ -343,7 +336,7 @@ contract Pricer is OpsManaged, PricerInterface {
uint256 _pricePoint,
uint256 _transferAmount,
uint256 _commissionAmount)
private
internal
view
returns (uint256, uint256) /* number of BT ,number of commission BT */
{
Expand All @@ -352,4 +345,88 @@ contract Pricer is OpsManaged, PricerInterface {
uint256 commissionAmountBT = SafeMath.div(SafeMath.mul(_commissionAmount, adjConversionRate), _pricePoint);
return (amountBT, commissionAmountBT);
}

/// @dev Takes _beneficiary, _transferAmount, _commissionBeneficiary, _commissionAmount;
/// checks if the current price point is in the acceptable range of intendedPricePoint;
/// internal method;
/// @param _beneficiary beneficiary
/// @param _transferAmount transferAmount
/// @param _commissionBeneficiary commissionBeneficiary
/// @param _commissionAmount commissionAmount
/// @return bool isValid
function isValidBeneficiaryData(
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is ideal to put as up in Pricer.sol as a modifier

address _beneficiary,
uint256 _transferAmount,
address _commissionBeneficiary,
uint256 _commissionAmount)
internal
returns (bool /*isValid*/)
{
require(_beneficiary != address(0));
require(_transferAmount != 0);

if (_commissionAmount > 0) {
require(_commissionBeneficiary != address(0));
}
return true;
}

/// @dev Takes _spender, _beneficiary, _tokenAmount, _commissionBeneficiary, _commissionTokenAmount;
/// Perform tokenAmount transfer
/// Perform commissionTokenAmount transfer
/// internal method;
/// @param _spender spender
/// @param _beneficiary beneficiary
/// @param _tokenAmount tokenAmount
/// @param _commissionBeneficiary commissionBeneficiary
/// @param _commissionTokenAmount commissionTokenAmount
/// @return (bool)
function performTransfer(
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to performTransfers

address _spender,
address _beneficiary,
uint256 _tokenAmount,
address _commissionBeneficiary,
uint256 _commissionTokenAmount)
internal
returns (
bool /* boolean value */)
{
require(EIP20Interface(pricerBrandedToken).transferFrom(_spender, _beneficiary, _tokenAmount));

if (_commissionBeneficiary != address(0)) {
require(EIP20Interface(pricerBrandedToken).transferFrom(_spender,
_commissionBeneficiary, _commissionTokenAmount));
}
return true;
}

/// @dev Takes _currency, _intendedPricePoint, _transferAmount, _commissionAmount;
/// Validate accepted margin
/// Calculates tokenAmount and commissionTokenAmount
/// internal method
/// @param _currency currency
/// @param _intendedPricePoint intendedPricePoint
/// @param _transferAmount transferAmount
/// @param _commissionAmount commissionAmount
/// @return (pricePoint, tokenAmount, commissionTokenAmount)
function validateMarginAndCalculateBTAmount(
bytes3 _currency,
uint256 _intendedPricePoint,
uint256 _transferAmount,
uint256 _commissionAmount)
internal
returns (uint256, uint256, uint256) /* pricePoint, tokenAmount, commissionTokenAmount */
{
uint256 pricePoint = getPricePoint(_currency);
require(pricePoint > 0);
require(isPricePointInRange(_intendedPricePoint, pricePoint, acceptedMargins(_currency)));

uint256 tokenAmount;
uint256 commissionTokenAmount;
(tokenAmount, commissionTokenAmount) = getBTAmountFromCurrencyValue(pricePoint, _transferAmount,
Copy link
Contributor

Choose a reason for hiding this comment

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

this function call to getBTAmountFromCurrencyValue should be inlined (as demonstrated that it calls again for getPricePoint; and the function itself on L221 should be removed;

the functionality of having this function (L221) public is achievable in the interaction layer by using getPricePoint;

Copy link
Contributor Author

@abhayks1 abhayks1 Feb 15, 2018

Choose a reason for hiding this comment

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

as we concluded getPricePoint is called once in pay and payAirdrop flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

correction, getBTAmountFromCurrencyValue does not query getPricePoint; there is still value in looking to inline this function to reduce the stack depth;

in return getPricePointandCalculateAmounts can be moved to the interaction layer

Copy link
Contributor

@jasonklein jasonklein Feb 16, 2018

Choose a reason for hiding this comment

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

@benjaminbollen: are you noting this for posterity or commenting that this is a change you would like Abhay to make for this PR (which I think contradicts what we agreed in the meeting)? And does this relate to #50?

_commissionAmount);

return (pricePoint, tokenAmount, commissionTokenAmount);
}

}
Loading