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

fix(#patch); venus; handle proxy contracts update #2454

Merged

Conversation

dhruv-chauhan
Copy link
Collaborator

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
nit
Copy link
Contributor

@melotik melotik left a comment

Choose a reason for hiding this comment

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

@dhruv-chauhan I think I am confused how this change is going to fix the issue..?

  • You create the template for Ctoken and VtokenV2 for each and every market
  • and the events Mint() and Redeem() point to the same function twice
  1. Wouldn't the event signature be different and not even work for handleMint for the V2 contract?

If you look at what I did in compound V2:

CTokenOld.create(event.params.cToken);
we just point to the new template when it was first used.

What do you think of this? Maybe I am wrong

@dhruv-chauhan
Copy link
Collaborator Author

  • You create the template for Ctoken and VtokenV2 for each and every market

I think that's necessary, and should not be much overhead. I'm assuming MarketListed is not emitted again on ABI change.
I'm not sure how compound-v2's change would handle for ABI change in existing markets.

  • and the events Mint() and Redeem() point to the same function twice. Wouldn't the event signature be different and not even work for handleMint for the V2 contract?

Yes, the signatures will be different, but it works in this case since the event.params we are using inside the handlers are common for both signatures.
If that's not a good practice, I can create separate handlers for both versions.

@melotik

@melotik
Copy link
Contributor

melotik commented Jan 19, 2024

@dhruv-chauhan

I see, yeah I think I was getting confused bc comp v2 does not change the abi after it has been deployed. It simply just as a new abi for newer markets. So this seems like an accurate fix.

As for the question about using the same function for both ABIs, to me that is kind of confusing. But, it is not a huge deal. In an effort to not add more indexing time, I think we can leave it as is.

Approving, but make sure to fix the prettier stuff before merging :)

@dhruv-chauhan dhruv-chauhan force-pushed the venus-proxy-contracts-update branch from da04b8a to a35ebed Compare January 23, 2024 11:59
@dhruv-chauhan dhruv-chauhan force-pushed the venus-proxy-contracts-update branch from d1d8edb to 0393bbb Compare January 23, 2024 12:16
@dhruv-chauhan
Copy link
Collaborator Author

Not sure why prettier is still failing. Failing check is passing fine for me,

prettier --check "subgraphs/compound-forks/protocols/venus/src/mapping.ts" "subgraphs/compound-forks/src/mapping.ts"
Checking formatting...
All matched files use Prettier code style!

@melotik
Copy link
Contributor

melotik commented Jan 23, 2024

Not sure why prettier is still failing. Failing check is passing fine for me,

@dhruv-chauhan I get the same thing. We can merge this to unblock for now

@dhruv-chauhan dhruv-chauhan merged commit e5db07d into messari:master Jan 25, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#bug; Venus Protocol Minting Issue #bug; UNI asset is missing in for Venus
2 participants