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

System entry point #793

Merged
merged 8 commits into from
Feb 24, 2021
Merged

System entry point #793

merged 8 commits into from
Feb 24, 2021

Conversation

ethanfrey
Copy link
Member

Closes #693

@ethanfrey
Copy link
Member Author

Okay, rebased after latest fix to the test contract.
This should be done now and is ready for review

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Looks good.

I just wonder if there is a good verb isntead of "system". We use verbs for the other calls as well (migrate, query, init). But I don't have a great idea right now.

pub struct SystemMsg {
pub recipient: HumanAddr,
pub amount: Vec<Coin>,
}
Copy link
Member

Choose a reason for hiding this comment

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

What about structuring this as an enum, such that this gets a verb like SystemMsg::StealFunds as well as extensibility.

Copy link
Member Author

@ethanfrey ethanfrey Feb 24, 2021

Choose a reason for hiding this comment

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

As an example sure.

I am happy to rename it, but it is designed to allow some priviledged "system" calls as opposed to external calls. Unless you have a better name, I would like to merge and then we can rename before the 0.14 release when we have a good name.

Copy link
Member

@webmaster128 webmaster128 Feb 24, 2021

Choose a reason for hiding this comment

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

As an example sure.

Yeah, I think it is easier for readers to habve a one-case enum than trying to wrap their head around a struct/enum mix.

I am happy to rename it, but it is designed to allow some priviledged "system" calls as opposed to external calls. Unless you have a better name, I would like to merge and then we can rename before the 0.14 release when we have a good name.

Agreed, let's merge as it. Will think about it.

Copy link
Contributor

@maurolacy maurolacy Feb 24, 2021

Choose a reason for hiding this comment

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

Sorry, I missed this. Some late comments.

Regarding naming:
Maybe indicating the priviledged nature of the call in the name would be a good idea. Priviledged sounds bad, but maybe Admin, Administer, or even Root are good alternatives to System.

System / Admin for the messages / enums, and administer / root as a function might work.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we can root the contracts? I like it 😄

Copy link
Member

Choose a reason for hiding this comment

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

oO bad things showing up in the dictionary.

But looking the root/admin direction, what about sudo. It is short, and a verb.

Copy link
Contributor

@maurolacy maurolacy Feb 25, 2021

Choose a reason for hiding this comment

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

I like it. It means "superuser do", which gives us another option: superuser. But I think sudo is better, as it captures the action.

@ethanfrey
Copy link
Member Author

Okay, just pushed a commit to address the SystemMsg enum.

@webmaster128 webmaster128 added the automerge See mergify.io label Feb 24, 2021
@mergify mergify bot merged commit ee76be0 into main Feb 24, 2021
@mergify mergify bot deleted the system-entry-point branch February 24, 2021 12:27
@maurolacy
Copy link
Contributor

maurolacy commented Feb 24, 2021

This looks good to me, and I think something like this will be necessary / useful, but I worry a bit about how it wil be controlled / secured.

@maurolacy
Copy link
Contributor

Whitelists, I guess, or something like the Admin module? Maybe you can comment a little about it.

@webmaster128
Copy link
Member

This looks good to me, and I think something like this will be necessary / useful, but I worry a bit about how it wil be controlled / secured.

The handle entry point is only executed because the chain calls it when seeing a transaction with a MsgEcecuteContract. The migrate entry point is only executed because the chain calls it when seeing a transaction with a migrate message and checked the sender address is the admin address. And here, the system is only executed whenever the blockchain decides to. This may be a result of some transaction type, but it can also be based on other events like at the beginning of each block.

What all cases have in common is the contract is never executed by the end user directly. Only the functionality explicitly exposed to the user is available.

@maurolacy
Copy link
Contributor

maurolacy commented Feb 24, 2021

Thanks. So, system basically is an internal function, and not an entry point. Plus, it's not available to contracts / contract devs.

@webmaster128
Copy link
Member

Not quite. All entry points are entry points to the contract, but not all of them are accessible to the user. The flow looks like this:

handle: User --> Transaction --> x/wasm --> wasmvm --> cosmwasm-vm --> contract

where x/wasm is the primary gate keeper. Now there can be other modules next to x/wasm requesting system calls:

handle: User --> Transaction --> x/wasm --> wasmvm --> cosmwasm-vm --> contract
                                   ^
                                   |
                                   |
system:                        x/whatever

@maurolacy
Copy link
Contributor

maurolacy commented Feb 25, 2021

Great diagram, thanks. When you write Transaction, it means a blockchain transaction, x/wasm <-> Cosmos SDK interaction, etc., etc.

What happens with the messages generated / returned by a contract, from this new system call, and from handle? What is the flow / what are the differences?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge See mergify.io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add optional system entry point
3 participants