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: create an append_message_output method #590

Merged
merged 9 commits into from
Sep 26, 2022

Conversation

iqdecay
Copy link
Contributor

@iqdecay iqdecay commented Sep 22, 2022

Closes #579 by creating an append_message_output method that works similarily
to append_variable_output.

@iqdecay iqdecay requested a review from a team September 22, 2022 12:10
@iqdecay
Copy link
Contributor Author

iqdecay commented Sep 22, 2022

@mohammadfawaz I'm not sure I understood the context for this so I didn't include Sway tests as I would usually do, do you have some Sway code to show me where this API could interact?

@nfurfaro
Copy link

nfurfaro commented Sep 22, 2022

@iqdecay 2 points:
1.) My draft PR is just waiting for this PR to land and be released so I can test it correctly!
Similar to how the tro(transfer to output) opcode requires that a variable output be appended in order to work, the smo opcode (send message output) requires a message output be appended to the call in order to work.

2.) we should consider if there's any other way to approach testing, as we've already seen that there's a circular dep issue between the stdlib (the tests use the SDK) and the SDK (which has some tests which use sway (and the stdlib).
It might be that we work on features like this one in tandem, and you can run your tests pointing at a stdlib branch other than master where the supporting changes have been made already. Once the SDK changes are released, the stdlib changes can merge to master, and then the SDK tests can be updated to use the master stdlib branch...

Open to ideas here, but the above approach would (imo) be better than the alternative, which is temporarily disabling tests and related functionality in the SDK during this process of re-syncing dependencies.

Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

Good work! I left a nit and a question..

@iqdecay iqdecay requested review from segfault-magnet, hal3e and a team September 26, 2022 10:48
@iqdecay iqdecay requested a review from a team September 26, 2022 11:33
@iqdecay iqdecay merged commit 0972f1d into master Sep 26, 2022
@iqdecay iqdecay deleted the iqdecay/feat-append-message-output branch September 26, 2022 11:51
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.

Create an append_message_output method that is similar to append_variable_output
5 participants