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

[feature] Single Asset Vault #10

Draft
wants to merge 16 commits into
base: vault-base
Choose a base branch
from
Draft

[feature] Single Asset Vault #10

wants to merge 16 commits into from

Conversation

thejohnfreeman
Copy link
Owner

@thejohnfreeman thejohnfreeman commented Oct 17, 2024

This PR exists for incremental review. It remade XRPLF#5147. The branch is based on a merge of:

Below is how to rebase this work when one of the above branches changes. If one of them is merged into develop, then generally it is best to wait until Greg merges develop into his feature/mpt-stissue branch.

# First, create the vault-base branch.
git fetch gregt feature/mpt-stissue
git fetch jfreeman levels
git checkout -b tmp gregt/feature/mpt-stissue
git merge jfreeman/levels
git checkout vault-base
# Make sure the old state is saved somewhere.
git branch old-vault-base
git reset --hard tmp
git push --force
git branch -D tmp
# Rebase the vault branch. Assumes you have the old vault-base saved in old-vault-base.
# Otherwise, you need to pick through the commit history to find it.
git checkout vault
git rebase --onto vault-base old-vault-base
# Test and then push
cupcake install
.install/bin/rippled --unittest ripple.tx.Vault
git push --force
git branch -D old-vault-base


struct Subcase
{
Context& _;
Copy link

Choose a reason for hiding this comment

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

please do not use _ as a variable or data member name; C++ is about to get special meaning for it https://wg21.link/P2169

Copy link

Choose a reason for hiding this comment

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

@@ -90,6 +92,9 @@ class Asset
void
setJson(Json::Value& jv) const;

STAmount
Copy link

Choose a reason for hiding this comment

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

can this be explicit ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's a function call operator. What is there to make explicit?

Copy link

Choose a reason for hiding this comment

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

Doh. For some reason I thought this to be conversion. NVM :)

@@ -189,6 +189,9 @@ enum LedgerSpecificFlags {

// ltCREDENTIAL
lsfAccepted = 0x00010000,

// ltVAULT
lsfVaultPrivate = 0x00010000,
};
Copy link

Choose a reason for hiding this comment

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

I guess we do not have flag lsfShareNonTransferable because, for this purpose, we will rely on lsfMPTCanTransfer instead ?

Copy link

Choose a reason for hiding this comment

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

also need flag lsfFrozen

Copy link
Owner Author

Choose a reason for hiding this comment

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

Answer to lsfShareNonTransferable: correct. Please refer to the spec.

Answer to lsfFrozen: there is no vault freeze, thus no lsfFrozen.

ReadView const& view,
std::shared_ptr<SLE> const& vault,
STAmount const& shares);

Copy link

Choose a reason for hiding this comment

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

I am confused by the use of STAmount and Number in the above three function signatures. I would assume that when we return take/shares, it would be STAmount (but that's not correct) or when we take/return assets that would be ... Number ?? but that's also not correct.

Copy link
Owner Author

Choose a reason for hiding this comment

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

When I first wrote these, they returned Number. They had no callers. Now that I'm calling them, I want STAmount. I've switched the first one now that it has a caller. I expect to switch the others when I write a call for them.

@@ -785,6 +792,9 @@ Env::rpc(std::string const& cmd, Args&&... args)
std::forward<Args>(args)...);
}

extern std::string blob257;
Copy link

@Bronek Bronek Nov 14, 2024

Choose a reason for hiding this comment

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

please add comment what these things are (e.g. hexadecimal blobs equivalent to N many bytes)

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