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

Pass the source account into the host and allow calling into the token wrapper host functions #3553

Merged
merged 7 commits into from
Sep 28, 2022

Conversation

sisuresh
Copy link
Contributor

@sisuresh sisuresh commented Sep 26, 2022

Description

Resolves #3546 and #3547.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@sisuresh sisuresh changed the title Pass source into the host Pass the source account into the host Sep 26, 2022
@sisuresh sisuresh requested a review from dmkozh September 27, 2022 00:11
Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

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

LGTM, modulo a couple nitpicks

// verify contract code is correct
LedgerTxn ltx2(app.getLedgerTxnRoot());
auto ltxe2 = loadContractData(ltx2, contractID, wasmKey);
REQUIRE((bool)ltxe2 == expectEntry);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please use static_cast - c-style casts should normally be avoided in c++.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
case HostFunction::HOST_FN_CREATE_CONTRACT_WITH_ED25519:
{
// uint256 salt = sha256("salt");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sisuresh sisuresh marked this pull request as draft September 28, 2022 16:10
@sisuresh
Copy link
Contributor Author

Marked as draft so we can get #3547 in as well.

@sisuresh sisuresh marked this pull request as ready for review September 28, 2022 16:55
@sisuresh sisuresh changed the title Pass the source account into the host Pass the source account into the host and allow calling into the token wrapper host functions Sep 28, 2022
@sisuresh sisuresh requested a review from graydon September 28, 2022 19:09
host.invoke_function(hf, args)?;
}
HostFunction::CreateContract => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important to do now but the only thing differing between these arms is what gets logged, and these enum values all have debug representations -- you can just print that into the log and use a single branch here, don't need to switch on the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Fixed.

Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

One nit I notice, everything else looks reasonable from what I understand about the goal here. I haven't been following the development of these APIs very closely though, so might want second eyes if you're looking for design feedback!

@graydon
Copy link
Contributor

graydon commented Sep 28, 2022

r+ 060a6bb

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.

Source Account Auth: Pass source account into the host in InvokeHostFunctionOp
4 participants