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(unmerged): contract caller as a chain extension origin #301

Open
wants to merge 117 commits into
base: main
Choose a base branch
from

Conversation

chungquantin
Copy link
Collaborator

@chungquantin chungquantin commented Sep 21, 2024

In the extension code, we have set the contract itself as the default origin for all calls. However, when it comes to methods like approve and transfer_from, there's a potential problem. Typically, the approve function allows a user (like ALICE) to approve another entity (like a contract) to spend their funds. But since we've set the contract as the origin for all calls, transfer_from would only be callable by the contract itself.

// Contract is the origin by default.
let origin = RawOrigin::Signed(env.ext().address().clone());
log::debug!(target: Logger::LOG_TARGET, "contract origin: origin={origin:?}");
let mut origin: Config::RuntimeOrigin = origin.into();

Shouldn't it be self.ext().caller() instead so the caller of the extrinsic is the caller of the contract, not the contract by itself. Would love to have some view on this!

Proposed changes

Payload sent to extension must have an extra flag to distinguish between which method should be called with contract as origin or caller as origin. Maybe the last byte of the encoding scheme?

Guideline for the reviewers

  • See transfer_from_works() in pop-api/examples/fungibles how ALICE can be the dispatch origin for transfer_from instead of a hard-coded contract address. We did not cover this in the contract integration test because it we use the dispatch functions from pallet-assets directly to handle these logics. But as a smart contract developer, everything must be possibly handled on the contract side.

Daanvdplas and others added 30 commits May 19, 2024 17:38
# This is the 1st commit message:

refactor: general

# This is the commit message #2:

init

# This is the commit message #3:

begin refactor

# This is the commit message #4:

refactor: error handling

# This is the commit message #5:

tests: add error handling tests

# This is the commit message #6:

WIP

# This is the commit message #7:

finalise error handling

# This is the commit message #8:

refactor: easier review
@chungquantin chungquantin added bug Something isn't working question Further information is requested extension labels Sep 21, 2024
@chungquantin chungquantin changed the title fix: extension origin fix: contract caller as a chain extension origin Sep 21, 2024
@chungquantin chungquantin requested review from evilrobot-01, Daanvdplas and peterwht and removed request for evilrobot-01 and Daanvdplas September 21, 2024 03:36
@chungquantin chungquantin self-assigned this Sep 21, 2024
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

I think this is definitely something that we need to consider. Both serve a different use case. When we use the contract address to make the calls, the contract serves somewhat as a different entity (e.g. a DAO). When we use the caller's address, the contract serves somewhat as a wrapper (e.g. a wrapper to make something standard compliant). Not sure how we can provide this different use case for each api. Creating a different function id sounds a bit like an overkill to differ between calling with the contract's address or the caller to the contract.

Providing the account which to call the pallet from was another idea but then we would have to filter out these bytes before decoding into the dispatch call or runtime read so that is not ideal either.

@@ -168,14 +171,14 @@ pub trait Ext {
type AccountId;

/// Returns a reference to the account id of the current contract.
fn address(&self) -> &Self::AccountId;
fn address(&self) -> Self::AccountId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we changing this?

}

impl Ext for () {
type AccountId = ();

fn address(&self) -> &Self::AccountId {
&()
fn address(&self) -> Self::AccountId {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, why are we changing this?

Copy link
Collaborator Author

@chungquantin chungquantin Sep 23, 2024

Choose a reason for hiding this comment

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

This was changed just to test if it works (will be reverted later when we decide the next step with the extension origin). Reason why I don't add the method caller() because it will bring back stuffs that @evilrobot-01 removed before #265 that removed the Config associated type. The caller() method returns OriginFor<Config> so if we want that method, we need to revert the changed Frank made.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be clear, I'm not against adding stuff that is needed, but a useful thing to consider is that taking a trait bound on the whole Config trait makes mocking overly complex. An example is needing to add a mock pallet-contracts impl just to do basic things.

Only expose what is required, nothing more. It may be easier, but has consequences downstream for implementors.

@@ -185,12 +188,16 @@ pub(crate) struct ExternalEnvironment<'a, T: pallet_contracts::chain_extension::
impl<'a, E: pallet_contracts::chain_extension::Ext> Ext for ExternalEnvironment<'a, E> {
type AccountId = AccountIdOf<E::T>;

fn address(&self) -> &Self::AccountId {
self.0.address()
fn address(&self) -> Self::AccountId {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not add a function caller instead? This function is to get the address and should not be changed tmo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Explained here: #301 (comment)

@chungquantin chungquantin changed the title fix: contract caller as a chain extension origin fix(unmerged): contract caller as a chain extension origin Sep 23, 2024
@chungquantin chungquantin removed bug Something isn't working ready-for-high-level-review labels Sep 23, 2024
@chungquantin chungquantin force-pushed the chungquantin/feat-psp22_example branch from 411ff66 to 8c72e2c Compare November 1, 2024 04:18
Base automatically changed from chungquantin/feat-psp22_example to main November 7, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked extension question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants