ScopeLift / gitcoin-web

🤖Grow Open Source
https://gitcoin.co
Other
1 stars 0 forks source link

POST request setup #40

Closed mds1 closed 4 years ago

mds1 commented 4 years ago

Currently a WIP. Once complete will close #35

Current status

Problems / Questions

  1. For the "Gitcoin Requests", the requests fail with 404 Not Found. This is expected because currently for these donations there is no grant ID or grant slug, and therefore we current POST to the non-existant endpoint /grants///fund. Can these be skipped, and if not how should these be handled?

  2. For the "Standard Requests", the POST calls all succeed. But there's a few questions to answer here:

    1. We need to confirm that the parameters in saveSubscriptionPayload are correct. Just search for this variable name in cart.js and you'll find the object.
    2. We need to confirm that the parameters in saveSplitTxPayload are correct. Just search for this variable name in cart.js and you'll find the object.
    3. It seems the saveSplitTx request is dependent on the saveSubscription request. Previously this was ok because of the multiple transactions. However, sending these simultaneously can fail if saveSubscription does not create the subscription object before saveSplitTx tries to access it. Potential solutions:
      1. Send saveSplitTx after saveSubscription returns. Downside: saveSubscription takes ~1 minute to return, so this is pretty bad UX, and if the browser is closed the request will not be sent (this is what is currently implemented).
      2. Send saveSubscription immediately before the first approval tx prompt, and send saveSplitTx after the final bulk donation tx is sent. This seems to be similar what is currently done, and the time delay seems to be sufficient to prevent saveSplitTx from erroring
      3. Some other TBD solution
mds1 commented 4 years ago

Hey @danlipert and @thelostone-mc, could you review this and let us know about the questions posted above? Thanks!

thelostone-mc commented 4 years ago

The flow description looks sane! Get All grants in cart -> form data -> invoke bulk contract -> get contract has -> loop through every grant in cart and make POST requests.

For the "Gitcoin Requests", the requests fail with 404 Not Found.

What do you mean by Gitcoin Request ? Are you referring to when an optional contributions is made to the gitcoin grant ?

mds1 commented 4 years ago

To clarify what I mean by "Gitcoin Requests", say the cart looks like the cart below. The three items shown in the cart are what I was calling Standard Requests. You can see the gitcoin donation amount is 5%, so 2 additional donations will be automatically generated:

Since those two were not explicit donations chosen by the user, do we want to POST those two additional donations (the "Gitcoin Requests")? If so, we can pull the grant data from the Gitcoin Development Fund grant and use that, or we can just skip these POST requests for the automatically generated donation to Gitcoin

image

danlipert commented 4 years ago

For the Gitcoin requests, you can save them as contributions to this grant: https://gitcoin.co/grants/12/gitcoin-open-source-support-fund right @owocki?

owocki commented 4 years ago

yar

mds1 commented 4 years ago

@owocki @danlipert The link currently seen on the cart page (which pulls from the existing fund page) links to this grant: https://gitcoin.co/grants/86/gitcoin-sustainability-fund

Just want to confirm that grand ID 12 linked by Dan is the correct one, as opposed to a new one equivalent to ID 86, e.g. "Gitcoin Grants Round 7+ Development Fund"

owocki commented 4 years ago

hmmm actually lets use https://gitcoin.co/grants/86/gitcoin-sustainability-fund - thanks for asking

thelostone-mc commented 4 years ago

Okay so would I be right in saying there would be 3 txn in the example you showed out ?

  1. bulk contract invocation 500 DAI + 250 DAI (or would these be 2 txns?)
  2. 1 time contribution of (0.1 + 0.01) * 0.05 DAI to Gitcoin Grant
  3. 1 time contribution of 0.75 * 0.05 ETH to Gitcoin Grant

2 ways to go about it in my head :

Option 1

======

well you could run the gitcoin grant contribution first -> and have the splitter hash as None which would return a reference to the contributions

once the bulk checkout data POST call is fired -> it would pass those references and the backend would update them

Option 2 EASIER WAY

======

do the transaction in any order but combine the saveSplitTxParams and saveSubscriptionPayload in one request AKA common payload and let the backend create the data accordingly

mds1 commented 4 years ago

There would only be 1 or 2 transactions in that example:

  1. Tx 1: Approve the BulkCheckout contract to spend our DAI (not necessary if user already gave sufficient approval amounts in a previous donation tx)
  2. Tx 2: One call to the BulkCheckout contract that does all the donations in one transaction. This includes the 3 standard donations shown in the cart and the 2 extra donations to the Gitcoin grant

However, I think Option 2 would be ideal either way and can test that out shortly

thelostone-mc commented 4 years ago

Ah then your point on saveSplitTxParams being redundant makes sense !! Let's sync up with @danlipert on discord and see if this is something we could remove for payout

mds1 commented 4 years ago

Just tested this out by combining the payloads and doing one API call, and the requests succeeded. So @danlipert, can you confirm the following combined payload and parameters look correct, and if not what should be changed?

The TBD comments are the particular fields I want to confirm, though it's probably best to confirm all of them. The TBD comments refer to the line immediately below the comment.

const saveSubscriptionPayload = new URLSearchParams({
  admin_address: donation.grant.grant_admin_address,
  amount_per_period: Number(donation.grant.grant_donation_amount),
  comment,
  // TBD: Is this needed? If so who/when/how should it be updated in DB?
  confirmed: false,
  contract_address: donation.grant.grant_contract_address,
  contract_version: donation.grant.grant_contract_version,
  contributor_address: userAddress,
  csrfmiddlewaretoken,
  denomination: tokenAddress,
  frequency_count: 1,
  frequency_unit: 'rounds',
  // TBD: Do we need a real value here or is 0 ok?
  gas_price: 0,
  // TBD: Confirm this field is the percentage to Gitcoin. If so, what value should
  // we use when this is one of the automatically generated "Gitcoin Request" 
  // donations? Zero?
  'gitcoin-grant-input-amount': this.gitcoinFactorRaw,
  gitcoin_donation_address: gitcoinAddress,
  grant_id: grantId,
  hide_wallet_address: this.hideWalletAddress,
  match_direction: '+',
  network,
  num_periods: 1,
  real_period_seconds: 0,
  recurring_or_not: 'once',
  // TBD: Set to empty string or 'onetime'?
  signature: 'onetime',
  // TBD: This txhash is our bulk donation hash, confirm this is what we want
  split_tx_id: txHash,
  splitter_contract_address: bulkCheckoutAddress,
  // TBD: This txhash is our bulk donation hash, confirm this is what we want
  sub_new_approve_tx_id: txHash,
  // TBD: Set to empty string or 'onetime'?
  subscription_hash: 'onetime',
  token_address: tokenAddress,
  token_symbol: tokenName
});
danlipert commented 4 years ago

@mds1comment is an internal field used for human readable description of the state of a subscription, not needed here gas_price represents the metatx gas price, if there is no metatx (i.e. eip 1337 recurring) then you don't need to worry about it (0 is fine) signature should be 'onetime' split_tx_id you could have the bulk donation hash here, or create a new field for this and leave this one empty. I think reusing it is probably fine sub_new_approve_tx_id is for the erc20 token approval tx - probably want to keep using this as we will need approvals for bulk checkout as well, correct? subscription_hash should be 'onetime' I think

mds1 commented 4 years ago

Thanks @danlipert, just a few follow up questions:

comment is an internal field used for human readable description of the state of a subscription, not needed here

I thought this field was where the comment to the grant owner goes. If that is not the case, where does that comment go? Related to https://github.com/ScopeLift/gitcoin-web/issues/32

split_tx_id you could have the bulk donation hash here, or create a new field for this and leave this one empty. I think reusing it is probably fine

I'll leave this as in then just to simplify integration, since we know it gets handled properly this way

sub_new_approve_tx_id is for the erc20 token approval tx - probably want to keep using this as we will need approvals for bulk checkout as well, correct?

Typically yes, but not always—it's possible that during the first checkout they approve MAX_UINT, and on subsequent checkout don't need another approval tx. It should be easy enough to update this to the approval tx if there is one, or an empty string if there isn't

danlipert commented 4 years ago

@mds1 ah yes - you are right about the comment, I was confusing it with the subminer_comments field. everything else sounds good 👍

mds1 commented 4 years ago

Awesome, thanks a lot @danlipert

mds1 commented 4 years ago

Closed by #71