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.

Sign up for GitHub

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

Jump to bottom

Asset: Separate preparation of creation of VC Asset from dispatch #215

Merged
merged 4 commits into from
Jul 10, 2024
Merged

Asset: Separate preparation of creation of VC Asset from dispatch #215

merged 4 commits into from
Jul 10, 2024

Conversation

masterismail
Copy link
Contributor

fixes #213

@masterismail
Copy link
Contributor Author

@vatsa287 @amarts please review.

vatsa287
vatsa287 requested changes Apr 30, 2024
View reviewed changes
Copy link
Member

@vatsa287 vatsa287 left a comment

Choose a reason for hiding this comment

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

Please keep a newline after every method end.

packages/asset/src/Asset.chain.ts
assetQty,
digest,
creator,
authorizationUri,
Copy link
Member

Choose a reason for hiding this comment

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

prepareVcExtrinsic expects authorAccount as a parameter after creator, which is missing while calling the method.

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 !

vatsa287
vatsa287 approved these changes Apr 30, 2024
View reviewed changes
Copy link
Member

@vatsa287 vatsa287 left a comment

Choose a reason for hiding this comment

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

LGTM

@vatsa287 vatsa287 requested a review from amarts April 30, 2024 05:41
amarts
amarts approved these changes Apr 30, 2024
View reviewed changes
Copy link
Member

@amarts amarts left a comment

Choose a reason for hiding this comment

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

Nitpick: A good PR should implement only focused area, and fix whitespace issues only in the related area. For fixing any other lines, either send them as a separate PR or separate commit in the same PR (ie, one PR can have multiple commits) so for reviewer, it is easier to focus on the 'logic' changes.

That practise helps in getting PRs reviewed quickly.

@amarts
Copy link
Member

amarts commented Apr 30, 2024

Ah! anyways, I see some merge conflicts here, can you rebase the whole PR ?

@masterismail
Copy link
Contributor Author

Nitpick: A good PR should implement only focused area, and fix whitespace issues only in the related area. For fixing any other lines, either send them as a separate PR or separate commit in the same PR (ie, one PR can have multiple commits) so for reviewer, it is easier to focus on the 'logic' changes.

That practise helps in getting PRs reviewed quickly.

Resolved the merged conflict, thank you for the feedback ! I'll keep PRs more focused in the future.

@VedantKhairnar
Copy link

@vatsa287 If the PR is good, can we merge it? Thanks

@amarts amarts merged commit ce90fc4 into dhiway:develop Jul 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amarts amarts amarts approved these changes

@vatsa287 vatsa287 Awaiting requested review from vatsa287

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C4GT] Asset: Separate preparation of creation of VC Asset from dispatch
4 participants
@masterismail @amarts @VedantKhairnar @vatsa287