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

add tecMPT_ISSUANCE_NOT_FOUND code #33

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

shawnxie999
Copy link

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)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@shawnxie999 shawnxie999 changed the title add issuance not found code add tecMPT_ISSUANCE_NOT_FOUND code Sep 6, 2024

mptAlice.create({.ownerCount = 1, .holderCount = 0});

mptAlice.authorize({.account = &bob});
Copy link
Owner

Choose a reason for hiding this comment

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

Don't need to authorize.

Copy link
Author

Choose a reason for hiding this comment

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

I think we should keep the test as it is, I added another test case where the holder didn't authorize

@@ -1875,7 +1875,7 @@ rippleCredit(
view.update(sle);
}
else
return tecINTERNAL;
return tecMPT_ISSUANCE_NOT_FOUND;
Copy link
Owner

Choose a reason for hiding this comment

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

If a sender is not the issuer, need to check if mpt issuance doesn't exist. I.e., if it doesn't exist then the error is tecMPT_ISSUANCE_NOT_FOUND and if mpt token doesn't exist then the error is tecNO_AUTH.

Copy link
Author

Choose a reason for hiding this comment

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

Could you clarify? The code currently does what you have described.

Copy link
Owner

Choose a reason for hiding this comment

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

If a sender is not the issuer then we are getting MPToken object. If this object doesn't exist then tecNO_AUTH is returned. This is not correct error if MPTokenIssuance object doesn't exist. I think we should check at the start of the function if MPTokenIssuance doesn't exist and return tecMPT_ISSUANCE_NOT_FOUND.

Copy link
Author

Choose a reason for hiding this comment

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

i moved the check to the start of function

@gregtatcam gregtatcam merged commit 8fd18a5 into gregtatcam:feature/mpt-v1 Sep 9, 2024
3 of 8 checks passed
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.

2 participants