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: Update to use the newest XDR defitinions #494

Merged
merged 7 commits into from
Mar 15, 2023

Conversation

Copy link
Contributor

@paulbellamy paulbellamy left a comment

Choose a reason for hiding this comment

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

Looking great so far!

cmd/soroban-cli/src/strval.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/strval.rs Show resolved Hide resolved
cmd/soroban-cli/src/strval.rs Outdated Show resolved Hide resolved
@willemneal
Copy link
Member Author

The last failing test is when trying to return a complex enum that includes a case with a tuple including a tuple struct.

To see the failing test pull the latest and run

cargo test --package soroban-cli --test it -- custom_types::complex_tuple_output --exact --nocapture

You should get:

-----------Result:
Vec(
    Some(
        ScVec(
            VecM(
                [
                    Symbol(
                        ScSymbol(
                            StringM(Tuple),
                        ),
                    ),
                    Vec(
                        Some(
                            ScVec(
                                VecM(
                                    [],
                                ),
                            ),
                        ),
                    ),
                ],
            ),
        ),
    ),
)

A new method was added to highlight this error:

    pub fn complex_tuple(_env: Env) -> ComplexEnum {
        ComplexEnum::Tuple(TupleStruct(
            Test {
                a: 1,
                b: true,
                c: Symbol::short("test"),
            },
            SimpleEnum::First,
        ))
    }

At first I thought this was a bug in the enum parsing on my end, but this is the value that is returned from the call.

Note: you need to rebuild the examples with make build-test-wasms.

@graydon
Copy link
Contributor

graydon commented Mar 14, 2023

@willemneal apologies, a bit of a hard landing of a lot of new code and this was obviously a broken path we just didn't have a test for. fix is in stellar/rs-soroban-env#726 which should merge shortly.

@willemneal willemneal marked this pull request as ready for review March 14, 2023 20:21
@willemneal willemneal changed the title feat: initial work with new XDR feat: Update to use the newest XDR defitinions Mar 14, 2023
@willemneal
Copy link
Member Author

@paulbellamy Currently this depends on stellar/rs-soroban-sdk#886 so it shouldn't merge until then, and I'm getting some errors in CI because it currently depends on AhaLab's branch.

@paulbellamy paulbellamy changed the base branch from xdr-update to main March 15, 2023 14:33
@paulbellamy paulbellamy changed the base branch from main to xdr-update March 15, 2023 14:33
@paulbellamy
Copy link
Contributor

(changed base back-and-forth so github would stop showing a diff that was already in the base branch)

Copy link
Contributor

@paulbellamy paulbellamy left a comment

Choose a reason for hiding this comment

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

Missing test cases, and sdk bug aside, LGTM.

@@ -77,8 +71,8 @@ impl Contract {
complex
}

pub fn address(_env: Env, address: Address) -> Address {
address
pub fn addresse(_env: Env, addresse: Address) -> Address {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope an update from the sdk added a new implicit address method. Should I add a comment?

@willemneal
Copy link
Member Author

I forgot this was pointing at a different branch. I guess we have less stress merging it. Then we could have another PR to add the tests and then merge to main? Or should we just change the branch to main now?

@paulbellamy
Copy link
Contributor

Yeah, let's do it. So long as we don't forget the fixes.

@paulbellamy paulbellamy merged commit 14dfd70 into stellar:xdr-update Mar 15, 2023
paulbellamy pushed a commit that referenced this pull request Mar 21, 2023
* feat: Update to use the newest XDR defitinions (#494)

* fix: add back type def parsing for return val

* fix: switch to eprintln

* chore: narrowed down failing case

* fix: update to env fix

* chore: clean up test wasm

Removed unneeded test wasm methods.

* chore: remove unneeded comment

* fix: removed unneeded unknown error

* fix(cli): symbol (#522)

* Fix simulate-transaction test expected xdr. But that will depend on go xdr updates

* Update go.mod from stellar/go#4814

* Forgot go.sum

---------

Co-authored-by: Willem Wyndham <[email protected]>
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