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

feat(subgraph): Receipts improved #13824

Merged
merged 20 commits into from
May 14, 2024
Merged

feat(subgraph): Receipts improved #13824

merged 20 commits into from
May 14, 2024

Conversation

julien51
Copy link
Member

Description

We recently found that accessing the transaction's value was not accurate because the call to pruchaseKeys could be happening in a "larger" transaction with a different value.
In order to handle this, in the short term, we are looking at the GNPChange event's value.

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

Release Note Draft Snippet

@julien51 julien51 requested a review from clemsos May 10, 2024 15:33
@cla-bot cla-bot bot added the cla-signed label May 10, 2024
const txLog = logs[i]

if (
// txLog.address == UNLOCK_ADDRESS &&
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how to do that since we don't actually have Unlock's address here?

Copy link
Member

@clemsos clemsos left a comment

Choose a reason for hiding this comment

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

LG but good to add a test (tests are

describe('Receipts', function () {

// We cannot trust `event.transaction.value` because the purchase function could in fact
// be happening inside of a larger transaction whose value is not the amount transfered,
// In that case, we need to look up the GNPChanged event
if (logs) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems that part is only needed if the tx value is null

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I initially thought, but in fact the transaction.value could also be "wrong". Imagine if a safe tx buys 2 keys but starts with a 3rd tx in the buddle which funds the safe? Thent the value would be wrong

Copy link
Member

@clemsos clemsos May 10, 2024

Choose a reason for hiding this comment

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

Yes in the case of a safe or a multicall then the value can be calculated with the GNP.

Also for multiple purchases the tx.value is the total of all keys purchased, so using that will be wrong too

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, that is a very good point. For receipts we actually have a single receipt per transaction, even if it ends up minting multiple keys (similar to when you buy multiple things at once you only get a single receipt at the store).
So my approach of looking at the GNP is not great.

I guess I can loop over all the GNP changed events for now.

We really need an event to be triggered for receipt, like PurchaseReceipt() that would include the required details: payer, contract, totalPaid, hash... etc.

tokenAddress: Address,
refund: BigInt
): ethereum.TransactionReceipt {
return newTransactionReceipt([
Copy link
Member Author

Choose a reason for hiding this comment

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

Ohhh! That makes sense. I am not sure why I assumed there could be a single "log" per mocked transaction.

@clemsos
Copy link
Member

clemsos commented May 14, 2024

after some digging, the integration tests are failing because the subgraph is enable to access the logs topics of the GNPChanged event. The log are correctly emitted but for some reasons, the event emitted by other contracts that the one tracked by the graph "datasource" is empty -no values in topics, just the topics[0] hash.

nvm. When an event is fired from another contract, the topics are passed as data...

subgraph/src/receipt.ts Outdated Show resolved Hide resolved
@julien51 julien51 merged commit 9281f7e into master May 14, 2024
12 checks passed
@clemsos clemsos deleted the receipts-improved branch May 14, 2024 16:58
julien51 added a commit that referenced this pull request May 14, 2024
* adding comments

* using the GNP_CHANGED event to identify values in subgraphs

* undue change

* extracting the unlock address

* fixed wasm

* fixing API

* fixing mocks

* adding more comments

* add test case for GNPChanged

* better mock

* unit test ok

* use data field in receipt instead of topics

* Update subgraph/src/receipt.ts

* Update subgraph/tests/mockTxReceipt.ts

---------

Co-authored-by: Clément Renaud <clement@unlock-protocol.com>
Co-authored-by: Clément Renaud <clement+git@clementrenaud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants