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

internal/integration: update the soroban tests for auth next support #4759

Merged
merged 11 commits into from
Feb 9, 2023

Conversation

sreuland
Copy link
Contributor

@sreuland sreuland commented Feb 6, 2023

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

recompiled the wasms with latest upcoming version of rs-soroban-sdk which includes the auth next.
moved the contract source code for test fixtures into integration tests as well for full control.

Why

the soroban contract tests won't work until they are recompiled with wasm that is compliant with latest that core expects for auth next invocations.

Closes: #4757

Known limitations

@sreuland sreuland marked this pull request as ready for review February 6, 2023 23:24
@sreuland
Copy link
Contributor Author

sreuland commented Feb 7, 2023

contract fn invoke tests are failing at present, much of lower stack is changing atm, env/sdk, need to update the contract test fixtures here to ref the latest rs-soroban-sdk version and also pull the new soroban core with same rs-soroban-env for integration tests to run.

@sreuland sreuland requested review from 2opremio and tamirms February 7, 2023 06:19
@@ -0,0 +1,15 @@
### Contract test fixture source code
#### anytime contracct code changes, follow these steps to rebuild the test wasm fixtures:
Copy link
Contributor

@tamirms tamirms Feb 7, 2023

Choose a reason for hiding this comment

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

I think this can be automated using go generate and go embed. Checkout https://github.com/stellar/starbridge/pull/102/files#diff-e27b4787f7b159227c12040f2ed88c1010e737e596b05418831f0b42c4d8243e (specifically, gen.go ). You can add a go file in this directory with the following contents:

package contracts

import _ "embed"

//go:generate sh -c "cargo build --target wasm32-unknown-unknown --release"

//go:embed target/wasm32-unknown-unknown/release/add_u64.wasm
var AddU64Wasm []byte

This way all the contract code will be built via the go generate command (which we already run to generate xdr in the repo).

Copy link
Contributor

@2opremio 2opremio Feb 7, 2023

Choose a reason for hiding this comment

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

I don't think that will work because AFAIC you cannot embed files in parent directories (and the current directory for go:embed is the source file's directory)

golang/go#46056

Copy link
Contributor

Choose a reason for hiding this comment

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

the target folder containing the wasm is a child directory of the contracts directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tamirms @2opremio , do we want to start pulling this type of soroban dependency into horizon, the build environment will need to have rust toolchain, we'll need to track down build pipelines on horizon, there's jenkins pipeline builder, and quickstart have builders that would need to update.

I think in the short term of release prep, I could look into something like a dockerfile, which dev can manually run to automate the steps to some degree, does the rust toolchain and cargo builds internally and copy the wasm's to the /testdata/ destination.

can do more enhanced approach such as gogenerate and change the external builder integrations if needed as follow-on tech debt?

@2opremio
Copy link
Contributor

2opremio commented Feb 7, 2023

Related: #4760

We should probably incorporate a test making use of auth-next

@sreuland
Copy link
Contributor Author

sreuland commented Feb 7, 2023

@2opremio , as part of footprint processing in sac_test.go, add_footprint(), it's submitting preflight to core's endpoint first and getting errors:

Messages:   	HostError
        	            	Value: Status(UnknownError(0))
        	            	
        	            	Debug events (newest first):
        	            	   0: "wrong number of args"

should that be happening still or has preflight check been changed to use a new endpoint on rpc instead?

@2opremio
Copy link
Contributor

2opremio commented Feb 8, 2023

@sreuland uhm, I am not sure what’s broken. I can take a look today.

However, note that AFAIU Core’s simulate transaction endpoint is going away, so we will need to replace it with soroban rpc at some point.

@2opremio
Copy link
Contributor

2opremio commented Feb 8, 2023

@sreuland the error is coming from changes in the token contract (which the tests aren't yet accounting for).

See stellar/rs-soroban-env@fc611e5#diff-762cdd179f5c2d24d87ffe8aec8070d12ecdfaf53af56c19d07dfdcaf77bdc29

For instance, mint() has changed from:

fn mint(
        e: &Host,
        admin: Signature,
        nonce: i128,
        to: Identifier,
        amount: i128,
    ) -> Result<(), HostError>;

to

fn mint(e: &Host, admin: Address, to: Address, amount: i128) -> Result<(), HostError>;

Note that it now has 3 arguments.

@2opremio
Copy link
Contributor

2opremio commented Feb 8, 2023

I started converting the arguments but I am not sure how to create an Address (I left a TODO), I will ask on Slack

@2opremio 2opremio force-pushed the 4757_auth_next_tests branch from 62727b8 to 4e51ae9 Compare February 8, 2023 15:03
@sreuland
Copy link
Contributor Author

sreuland commented Feb 8, 2023

I started converting the arguments but I am not sure how to create an Address (I left a TODO), I will ask on Slack

ok, that helps me narrow focus, I'll look at your TODO and proceed from there, thanks!

@sreuland
Copy link
Contributor Author

sreuland commented Feb 9, 2023

@2opremio , tests are back to green, this looks ready to go now.

Related: #4760

We should probably incorporate a test making use of auth-next

Yes, the tests now use that Auth, populating with xdr.ContractAuth for token contract invocations.

@sreuland
Copy link
Contributor Author

sreuland commented Feb 9, 2023

@2opremio @tamirms , tests are back in green. maybe we should merge at this point as #4757 criteria are met. I can capture the request for automation of wasm compilation that was raised here as follow on ticket.

@2opremio
Copy link
Contributor

2opremio commented Feb 9, 2023

I can capture the request for automation of wasm compilation that was raised here as follow on ticket.

Sure!

@sreuland sreuland merged commit 5a13648 into stellar:soroban-xdr-next Feb 9, 2023
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.

3 participants