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

Fixed NFTokenMinter field validation error in AccountSet Transaction #2201

Merged
merged 10 commits into from
Mar 1, 2023
Merged

Fixed NFTokenMinter field validation error in AccountSet Transaction #2201

merged 10 commits into from
Mar 1, 2023

Conversation

TusharPardhe
Copy link
Contributor

High Level Overview of Change

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

@TusharPardhe TusharPardhe mentioned this pull request Feb 5, 2023
Closed
7 tasks
@ckniffen
Copy link
Collaborator

ckniffen commented Feb 5, 2023

Can you add a test?

@TusharPardhe
Copy link
Contributor Author

Can you add a test?

Yup! added one

ckniffen
ckniffen previously approved these changes Feb 8, 2023
View reviewed changes
@mvadari
Copy link
Collaborator

mvadari commented Feb 9, 2023

Please update the changelog.

@mvadari
Copy link
Collaborator

mvadari commented Feb 17, 2023

Please update your PR name to be more descriptive - it will become the commit name once it's squashed and merged.

@TusharPardhe TusharPardhe changed the title Update accountSet.ts Fixed NFTokenMinter validation error Feb 18, 2023
@TusharPardhe TusharPardhe changed the title Fixed NFTokenMinter validation error Fixed NFTokenMinter field validation error in AccountSet Transaction Feb 18, 2023
@ckniffen
Copy link
Collaborator

This PR will also affect this bug. XRPLF/rippled#4404 (review). I think it is being set to this account when blank is it is treating it as a seed of "".

@JST5000
Copy link
Contributor

JST5000 commented Feb 28, 2023

Thanks for the PR and quick follow ups :)

@mvadari mvadari requested a review from ckniffen February 28, 2023 15:25
mvadari
mvadari previously approved these changes Feb 28, 2023
View reviewed changes
@JST5000 JST5000 merged commit 0fa5415 into XRPLF:main Mar 1, 2023
@ckniffen ckniffen mentioned this pull request Apr 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinr1234 justinr1234 justinr1234 left review comments

@JST5000 JST5000 JST5000 approved these changes

@mvadari mvadari mvadari approved these changes

@ckniffen ckniffen ckniffen approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@TusharPardhe @ckniffen @mvadari @JST5000 @justinr1234