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

Smart Contract transfer and execute #340

Merged
merged 10 commits into from
Oct 26, 2023
Merged

Conversation

popenta
Copy link
Contributor

@popenta popenta commented Oct 17, 2023

Implemented the factory based on the specifications.

@popenta popenta self-assigned this Oct 17, 2023
@popenta popenta marked this pull request as draft October 17, 2023 12:20
Base automatically changed from update-transaction-factories to feat/factories October 23, 2023 13:55
@popenta popenta marked this pull request as ready for review October 23, 2023 14:02
src/errors.ts Show resolved Hide resolved
src/smartcontracts/smartContract.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@andreibancioiu andreibancioiu left a comment

Choose a reason for hiding this comment

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

Very nice 🎉 🚀

src/smartcontracts/smartContract.ts Outdated Show resolved Hide resolved
src/tokens.spec.ts Show resolved Hide resolved
src/tokens.ts Outdated
}
}

export class TokenTransfer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we'll have an issue 🙈

https://github.com/multiversx/mx-sdk-js-core/blob/main/src/tokenTransfer.ts

Let's brainstorm how to tackle this. Perhaps we can define a new sub-package, next, or perhaps we should use namespacing instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using namespacing now. Let me know what you think about it.

src/tokens.ts Outdated Show resolved Hide resolved
}
}

function decodeUnsignedNumber(arg: Buffer): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be moved near the other codec functions? Can also stay here for now, actually. We'll move it later, when / if needed.

receiver = options.sender;
}
}
else if (numberOfTokens > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Else, without if (also for py). Also, formatting / (braces & newlines).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is now, it doesn't properly work without else if. The code checks if someone provides a native token amount and custom token transfers but does not check if neither is provided. And it shouldn't check because you can simply call a contract function without doing any transfers. We check if the numberOfTokens == 1, but then if we use a simple else, and numberOfTokens = 0, the else branch get's executed and we don't want that since we don't have any transfers.

bogdan-rosianu
bogdan-rosianu previously approved these changes Oct 26, 2023
this.token = token;
this.amount = amount;
}
export class NextTokenTransfer {
Copy link
Contributor

Choose a reason for hiding this comment

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

✔️

@popenta popenta merged commit 85a6d4a into feat/factories Oct 26, 2023
1 check passed
@popenta popenta deleted the transfer-and-execute branch October 26, 2023 11:25
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