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 payable on up constructor / initializer(...) #219

Merged
merged 2 commits into from
Jul 12, 2022
Merged

Conversation

Hugoo
Copy link
Contributor

@Hugoo Hugoo commented Jul 1, 2022

Todo:

  • ajouter sur le init

Done with @CJ42

@Hugoo Hugoo requested review from CJ42 and YamenMerhi July 1, 2022 15:09
@Hugoo Hugoo marked this pull request as ready for review July 4, 2022 08:22
Copy link
Member

@YamenMerhi YamenMerhi left a comment

Choose a reason for hiding this comment

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

Could you mark the initialize function of UniversalProfileInit as payable also?

Copy link
Member

@CJ42 CJ42 left a comment

Choose a reason for hiding this comment

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

We also need to allow this feature in th eproxy version.
This require some tests + probably the _initialize function of LSP0ERC725AccountInitAbstract and UniversalPofileInitAbstract payable too, so that the initialize(...) payable function can pass the value down into the inheritance.

Below are the actual functions in the proxy that setup the UP and ERC725Account on deployment:

function _initialize(address newOwner) internal virtual onlyInitializing {
OwnableUnset._setOwner(newOwner);
}

function _initialize(address newOwner) internal virtual override onlyInitializing {
LSP0ERC725AccountInitAbstract._initialize(newOwner);
// set key SupportedStandards:LSP3UniversalProfile
_setData(_LSP3_SUPPORTED_STANDARDS_KEY, _LSP3_SUPPORTED_STANDARDS_VALUE);
}

tests/UniversalProfile.test.ts Show resolved Hide resolved
@YamenMerhi
Copy link
Member

@CJ42 internal and private functions cannot be payable.
It's not about passing the value to the internal functions (logic), it's about making the contract receive value by initializing with sending value along the way.

@CJ42
Copy link
Member

CJ42 commented Jul 4, 2022

Ah thanks @YamenMerhi for the clarification. I did not know actually that internal and private cannot be payable.
So just need to add the tests for the contracts that are deployed behind proxy.

@CJ42 CJ42 changed the title feat: add payable on up construct feat: add payable on up constructor / initializer(...) Jul 8, 2022
@CJ42
Copy link
Member

CJ42 commented Jul 8, 2022

Thanks @Hugoo for this contribution! 🚀 😃
@YamenMerhi let me know when it's good to go and be merged.

Copy link
Member

@YamenMerhi YamenMerhi left a comment

Choose a reason for hiding this comment

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

Super ! @Hugoo @CJ42

@CJ42 CJ42 merged commit 2f8adbd into develop Jul 12, 2022
@CJ42 CJ42 deleted the deploy-funding branch July 12, 2022 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants