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: add RaveShareMintOAV0 details #8

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

thearyanag
Copy link
Contributor

@thearyanag thearyanag commented Jan 7, 2024

Verify my module

  • Module type: open action
  • Module name: RaveShareMintOAV0
  • Module address: https://polygonscan.com/address/0x410688fc60028C805Bdf8592A6504A0096927911#code
  • My module is immutable: yes
  • My module is using an upgradeable proxy: no
  • My module has been registered on the registry: yes
  • My module has been verified on the block explorer: yes
  • My module is a paid action so uses currencies: yes
  • Summary of my module: The module accepts payment in wMATIC and sends it to our platform address, the amount is calculated using chainlink feed and is equivalent to 0.000777 ETH + 10% , as soon as payment is done, an event is sent which triggers our server to mint NFT ( zora ) to the user address, the chain can also be set during intialize action.
  • I have updated the module type json file: yes

Copy link

height bot commented Jan 7, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@donosonaumczuk
Copy link
Member

Some comments:

  1. Any reason why processPublicationAction is not onlyHub restricted? Is that intended?
    function processPublicationAction(
        Types.ProcessActionParams calldata params
    ) external override returns (bytes memory) {
        // ...
    }
  1. Seems that these events are not used, in case you want to remove them:
    event Log(string variable, string message);
    event Log(string variable, address message);
    event Log(string variable, int message);

@thearyanag
Copy link
Contributor Author

hey @donosonaumczuk , 1/2 were for testing purpose, I have updated the PR with the updated contract address, which you can find at https://polygonscan.com/address/0x410688fc60028c805bdf8592a6504a0096927911#code

@donosonaumczuk
Copy link
Member

Great, looks good to me now @thearyanag

@joshstevens19
Copy link
Member

Needs a rebase please @thearyanag

@vicnaum
Copy link

vicnaum commented Jan 8, 2024

Instead of

        if (!MODULE_GLOBALS.isErc20CurrencyRegistered(currency)) {
            revert CurrencyNotWhitelisted();
        }

you can verify that MODULE_GLOBALS.verifyErc20Currency(currency) returns true - this will automatically register a currency if it wasn't done before (making the module use with other currencies more user-friendly)

@thearyanag
Copy link
Contributor Author

hey @joshstevens19 , did the rebases, check now

@thearyanag
Copy link
Contributor Author

hey @vicnaum , thanks for the suggestion, that line is intended as we want to start this as a piolet module and iterate based on user feedback , in the coming module we'll open up the support for all token.

@joshstevens19
Copy link
Member

we can merge this but you need to rebase it has conflicts with base @thearyanag

@thearyanag
Copy link
Contributor Author

hey @joshstevens19 , I did the rebase earlier , I suppose with the recent changes to the base, there were conflicts, I rebased it again, check now

@joshstevens19 joshstevens19 merged commit 0a18eaf into lens-protocol:master Jan 9, 2024
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.

4 participants