Airdrop Contract #49

abhayks1 · 2018-02-14T14:16:56Z

Pricer Contract Refactoring
Airdrop Contract Changes
PricerInterface Contract Changes

Flow

msg.sender will be worker address
airdropBudgetHolder will hold total airdrop amount
performAirdropTransferToSpender => Approves that contract can transfer on behalf of airdropBudgetHolder
[ ]performTransfer => approves that contract on behalf of user can do transfer to beneficiary and
commission beneficiary
Airdrop.sol line no 165 => Can't we transfer all _airdropAmount to _spender?

…rdrop

# Conflicts: # contracts/Workers.sol

Airdrop & Pricer Refactoring

benjaminbollen

please already review comments, and we'll discuss more tomorrow during the call

benjaminbollen · 2018-02-14T17:43:21Z

package.json

@@ -4,6 +4,7 @@
  "description": "",
  "dependencies": {
    "@openstfoundation/openst-notification": "1.0.0-beta.1",
+    "@ostdotcom/ost-price-oracle": "^1.0.0-beta.3",


openst-payments cannot have a dependency on ostdotcom/ost-price-oracle; we outlined the tasks for there to be a price oracle interface defined in openst-payments, and for testing we want a PriceOracleMock that fills that interface

As per discussion we will take in a different ticket.

benjaminbollen · 2018-02-14T17:52:38Z

contracts/Airdrop.sol

+      *  External functions
+     */
+    /// clean up or revoke airdrop contract
+    function remove()


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

benjaminbollen · 2018-02-14T17:52:56Z

contracts/Airdrop.sol

+        external
+        onlyAdminOrOps
+    {
+        selfdestruct(msg.sender);


an event before self-destruct is useful

adding event "Removed"

benjaminbollen · 2018-02-14T18:30:51Z

contracts/Airdrop.sol

+import "./Pricer.sol";
+
+
+contract Airdrop is Pricer, Workers {


Airdrop is not Workers

benjaminbollen · 2018-02-14T18:36:26Z

contracts/Airdrop.sol

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


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;

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

benjaminbollen · 2018-02-14T18:37:36Z

contracts/Airdrop.sol

+     *  Events
+    */
+    /// Event for AirdropPayment complete
+    event AirdropPayment(


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)

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

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?

@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).

benjaminbollen · 2018-02-14T19:11:19Z

contracts/PricerInterface.sol

@@ -1,3 +1,4 @@
+/* solhint-disable-next-line compiler-fixed */


There is no need for PricerINterface.sol, please remove

as we concluded we will keep it.

benjaminbollen · 2018-02-14T19:13:25Z

contracts/Pricer.sol

+
+        uint256 tokenAmount;
+        uint256 commissionTokenAmount;
+        (tokenAmount, commissionTokenAmount) = getBTAmountFromCurrencyValue(pricePoint, _transferAmount,


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;

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

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

@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?

benjaminbollen · 2018-02-14T19:16:33Z

contracts/Pricer.sol

-            _commissionAmount, _currency, _intendedPricePoint, pricePoint);      
-        return true;
+            _commissionAmount, _currency, _intendedPricePoint, pricePoint);
+        return (tokenAmount + commissionTokenAmount);
    }

    /// @dev    Takes _currency; 


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

benjaminbollen · 2018-02-14T19:18:36Z

contracts/Pricer.sol

@@ -1,3 +1,4 @@
+/* solhint-disable-next-line compiler-fixed */


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

as we concluded we will keep PricerInterface.sol

benjaminbollen · 2018-02-14T19:27:03Z

contracts/Pricer.sol

+    /// @param _commissionBeneficiary commissionBeneficiary
+    /// @param _commissionAmount commissionAmount
+    /// @return bool isValid
+    function isValidBeneficiaryData(


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

benjaminbollen · 2018-02-14T19:29:47Z

contracts/Pricer.sol

+    /// @param _commissionBeneficiary commissionBeneficiary
+    /// @param _commissionTokenAmount commissionTokenAmount
+    /// @return (bool)
+    function performTransfer(


rename to performTransfers

abhayks1 · 2018-02-15T15:17:01Z

@benjaminbollen Done with requested changes. Few points:

Airdrop.sol => https://github.com/OpenSTFoundation/openst-payments/blob/ben/gh6/airdrop/contracts/Airdrop.sol
line no 155. Do we need this? Can't we transfer all _airdropAmount to _spender?
Making isValidBeneficiaryData modifier gives below error on airdrop contract compiling:
./contracts/Airdrop.sol:114:38: Compiler error: Stack too deep, try removing local variables.
_intendedPricePoint, _transferAmount, _commissionAmount);
Your thoughts?
"also for formatting on events we've previously filled the line (different from function signatures)"
I didn't understand this point. Please explain with an example?

- Remove modifier isValidBeneficiaryData and replace it with function

benjaminbollen

LGTM

benjaminbollen · 2018-02-16T14:27:25Z

contracts/Airdrop.sol


    /*
     *  Events
    */
-    /// Event for AirdropPayment complete
+    /// Emit AirdropPayment Event
    event AirdropPayment(


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

benjaminbollen · 2018-02-16T14:30:04Z

contracts/Airdrop.sol

-        address _commissionBeneficiary,
-        uint256 commissionTokenAmount,
+        address indexed _beneficiary,
+        uint256 _transferAmount,


Note: for alignment with Payment event from tokenAmount to transferAmount; we can take input from Saas what the best set of return values;

benjaminbollen · 2018-02-16T15:46:03Z

contracts/Pricer.sol

+        onlyAdminOrOps
+    {
+        selfdestruct(msg.sender);
+        Removed(msg.sender);


Can we put the event before the selfdestruct; or does solidity change the order of execution?

Makes sense,
Since the contract is killed off and there is no one/nothing left to return after destroyed. It obliterated the next step.

…ment event Fire event before calling self destruct

benjaminbollen

Lgtm

benjaminbollen and others added 7 commits January 28, 2018 21:59

contracts: proposal for Workers.sol to manage active workers

5315d1e

contracts: (incomplete) proposal for Airdrop

93c1933

contracts: change return value to total paid for pay()

4b9d49f

Merge remote-tracking branch 'remotes/origin/ben/gh6/airdrop' into ai…

2cea8c7

…rdrop

Merge remote-tracking branch 'remotes/origin/develop' into airdrop

ab90149

# Conflicts: # contracts/Workers.sol

rebased with develop

33342ef

Airdrop Contract Changes

69c1a03

Airdrop & Pricer Refactoring

abhayks1 assigned benjaminbollen Feb 14, 2018

benjaminbollen suggested changes Feb 14, 2018

View reviewed changes

benjaminbollen reviewed Feb 14, 2018

View reviewed changes

PR Review Changes

870649c

Jason Banks and others added 4 commits February 15, 2018 17:16

Contracts: comment/uncomment for beneficiary validation modifier.

7059b15

Enabling Airdrop modifier for banks to test

d45e084

Requested PR Review Changes

340baa4

- Remove modifier isValidBeneficiaryData and replace it with function

Added _currency in AirdropPayment event

69c7bbb

benjaminbollen approved these changes Feb 16, 2018

View reviewed changes

Added tokenAmount and commissionTokenAmount in Payment and AirdropPay…

380c39c

…ment event Fire event before calling self destruct

benjaminbollen approved these changes Feb 17, 2018

View reviewed changes

benjaminbollen merged commit bc4a0d6 into develop Feb 17, 2018

abhayks1 deleted the airdrop branch March 22, 2018 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Airdrop Contract #49

Airdrop Contract #49

abhayks1 commented Feb 14, 2018

benjaminbollen left a comment

benjaminbollen Feb 14, 2018

abhayks1 Feb 15, 2018

benjaminbollen Feb 14, 2018

benjaminbollen Feb 14, 2018

abhayks1 Feb 15, 2018 •

edited

Loading

benjaminbollen Feb 14, 2018

benjaminbollen Feb 14, 2018

abhayks1 Feb 15, 2018

benjaminbollen Feb 14, 2018

abhayks1 Feb 15, 2018

abhayks1 Feb 15, 2018

jasonklein Feb 15, 2018 •

edited

Loading

benjaminbollen Feb 14, 2018

abhayks1 Feb 15, 2018

benjaminbollen Feb 14, 2018

abhayks1 Feb 15, 2018 •

edited

Loading

benjaminbollen Feb 15, 2018

jasonklein Feb 16, 2018 •

edited

Loading

benjaminbollen Feb 14, 2018

benjaminbollen Feb 14, 2018

abhayks1 Feb 15, 2018

benjaminbollen Feb 14, 2018

benjaminbollen Feb 14, 2018

abhayks1 commented Feb 15, 2018

benjaminbollen left a comment

benjaminbollen Feb 16, 2018

benjaminbollen Feb 16, 2018

benjaminbollen Feb 16, 2018

abhayks1 Feb 17, 2018

benjaminbollen left a comment

		@@ -1,3 +1,4 @@
		/* solhint-disable-next-line compiler-fixed */

Airdrop Contract #49

Airdrop Contract #49

Conversation

abhayks1 commented Feb 14, 2018

benjaminbollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhayks1 Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonklein Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhayks1 Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonklein Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhayks1 commented Feb 15, 2018

benjaminbollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen left a comment

Choose a reason for hiding this comment

abhayks1 Feb 15, 2018 •

edited

Loading

jasonklein Feb 15, 2018 •

edited

Loading

abhayks1 Feb 15, 2018 •

edited

Loading

jasonklein Feb 16, 2018 •

edited

Loading