-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[Work in Progress] add AccountPermission #5164
base: develop
Are you sure you want to change the base?
Changes from 2 commits
6bbe2ca
372cc5c
9310eae
5f5b265
15506e5
1e15774
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
//------------------------------------------------------------------------------ | ||
/* | ||
This file is part of rippled: https://github.com/ripple/rippled | ||
Copyright (c) 2024 Ripple Labs Inc. | ||
|
||
Permission to use, copy, modify, and/or distribute this software for any | ||
purpose with or without fee is hereby granted, provided that the above | ||
copyright notice and this permission notice appear in all copies. | ||
|
||
THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES | ||
WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF | ||
MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR | ||
ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES | ||
WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN | ||
ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF | ||
OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. | ||
*/ | ||
//============================================================================== | ||
|
||
#ifndef RIPPLE_PROTOCOL_PERMISSION_H_INCLUDED | ||
#define RIPPLE_PROTOCOL_PERMISSION_H_INCLUDED | ||
|
||
#include <xrpl/protocol/STTx.h> | ||
#include <optional> | ||
#include <string> | ||
#include <unordered_map> | ||
|
||
namespace ripple { | ||
|
||
/** | ||
* We have transaction type permissions and granular type | ||
* permissions. Since we will reuse the TransactionFormats to parse the | ||
* Transaction Permissions, we only define the GranularPermissionType here. | ||
*/ | ||
|
||
enum GranularPermissionType : std::uint32_t { | ||
gpTrustlineAuthorize = 65537, | ||
|
||
gpTrustlineFreeze = 65538, | ||
|
||
gpTrustlineUnfreeze = 65539, | ||
|
||
gpAccountDomainSet = 65540, | ||
|
||
gpAccountEmailHashSet = 65541, | ||
|
||
gpAccountMessageKeySet = 65542, | ||
|
||
gpAccountTransferRateSet = 65543, | ||
|
||
gpAccountTickSizeSet = 65544, | ||
|
||
gpPaymentMint = 65545, | ||
|
||
gpPaymentBurn = 65546, | ||
|
||
gpMPTokenIssuanceLock = 65547, | ||
|
||
gpMPTokenIssuanceUnlock = 65548, | ||
}; | ||
|
||
class Permission | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should probably delete copy and copy assignment constructors There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this still needs to be done? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just added |
||
{ | ||
private: | ||
Permission(); | ||
|
||
std::unordered_map<std::string, GranularPermissionType> | ||
granularPermissionMap; | ||
|
||
std::unordered_map<GranularPermissionType, TxType> granularTxTypeMap; | ||
|
||
public: | ||
static Permission const& | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about just making all members as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need to access private members in the functions, for example, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They should be also static. There should be only one instance of those maps. |
||
getInstance(); | ||
|
||
std::optional<std::uint32_t> | ||
getGranularValue(std::string const& name) const; | ||
|
||
std::optional<TxType> | ||
getGranularTxType(GranularPermissionType const& gpType) const; | ||
|
||
bool | ||
isProhibited(std::string const& name) const; | ||
}; | ||
|
||
} // namespace ripple | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -436,3 +436,16 @@ LEDGER_ENTRY(ltCREDENTIAL, 0x0081, Credential, ({ | |
{sfPreviousTxnID, soeREQUIRED}, | ||
{sfPreviousTxnLgrSeq, soeREQUIRED}, | ||
})) | ||
|
||
/** A ledger object representing permissions an account has delegated to another account. | ||
|
||
\sa keylet::accountPermission | ||
*/ | ||
LEDGER_ENTRY(ltACCOUNT_PERMISSION, 0x0082, AccountPermission, ({ | ||
{sfAccount, soeREQUIRED}, | ||
{sfAuthorize, soeREQUIRED}, | ||
{sfPermissions, soeREQUIRED}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to make this optional since it could be empty? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since being empty means deleting all the permissions, can we keep it as required? So that the user won't delete the permissions by accident when they mistakenly leave out the Permissions field. Giving an empty field is more clear that they want to delete all the permissions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an on-ledger object, not the transaction. It's empty if there is no permissions so it could be optional. Can save a bit of space this way, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. make sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gregtatcam when there's no permissions, can we erase the SLE? so that we can still keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean delete SLE? I don't think we should delete it if there is no permissions. There is a transaction for this. Do you see a problem with making it optional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's easier to just delete the ledger entry if there's no permission between two accounts. Is there any reason why should we keep it and just leave out the permissions field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is a precedent for this, is there? A transaction is used to delete a ledger entry. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is a hard requirement that a specific txn is needed to delete an object. Ex. unfunded offers can be cancelled implicitly, associated NFT offers are automatically deleted when the NFT itself is deleted (see #4346) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can delete it then. |
||
{sfOwnerNode, soeREQUIRED}, | ||
{sfPreviousTxnID, soeREQUIRED}, | ||
{sfPreviousTxnLgrSeq, soeREQUIRED}, | ||
})) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -447,6 +447,12 @@ TRANSACTION(ttCREDENTIAL_DELETE, 60, CredentialDelete, ({ | |
{sfCredentialType, soeREQUIRED}, | ||
})) | ||
|
||
/** This transaction type delegates authorized account specified permissions */ | ||
TRANSACTION(ttACCOUNT_PERMISSION_SET, 61, AccountPermissionSet, ({ | ||
{sfAuthorize, soeREQUIRED}, | ||
{sfPermissions, soeREQUIRED}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to make it optional since it could be empty? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Being empty means deleting all the permissions, so that the user won't delete the permissions by accident when they mistakenly leave out the Permissions field. So I think it's better to keep it as soeREQUIRED here in the transaction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But it's a required field in the transaction, isn't it? It could be an empty array of permissions when it's set. So if it's empty then we delete it from the ledger object. The optional field has to be handled appropriately but it's used in a very few places. So if a user can make the permissions array empty then why is it a problem deleting it from the ledger object? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's kept required in the transaction but optional in the on-ledger object. |
||
})) | ||
|
||
|
||
/** This system-generated transaction type is used to update the status of the various amendments. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,7 @@ | |
MPTOKEN_ISSUANCE = '~', | ||
MPTOKEN = 't', | ||
CREDENTIAL = 'D', | ||
ACCOUNT_PERMISSION = 'P', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This value is already used by |
||
|
||
// No longer used or supported. Left here to reserve the space | ||
// to avoid accidental reuse. | ||
|
@@ -428,6 +429,23 @@ | |
return {ltAMM, id}; | ||
} | ||
|
||
Keylet | ||
accountPermission( | ||
AccountID const& account, | ||
AccountID const& authorizedAccount) noexcept | ||
{ | ||
return { | ||
ltACCOUNT_PERMISSION, | ||
indexHash( | ||
LedgerNameSpace::ACCOUNT_PERMISSION, account, authorizedAccount)}; | ||
} | ||
|
||
Keylet | ||
accountPermission(uint256 const& key) noexcept | ||
{ | ||
return {ltACCOUNT_PERMISSION, key}; | ||
} | ||
|
||
Keylet | ||
bridge(STXChainBridge const& bridge, STXChainBridge::ChainType chainType) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
//------------------------------------------------------------------------------ | ||
/* | ||
This file is part of rippled: https://github.com/ripple/rippled | ||
Copyright (c) 2024 Ripple Labs Inc. | ||
|
||
Permission to use, copy, modify, and/or distribute this software for any | ||
purpose with or without fee is hereby granted, provided that the above | ||
copyright notice and this permission notice appear in all copies. | ||
|
||
THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES | ||
WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF | ||
MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR | ||
ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES | ||
WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN | ||
ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF | ||
OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. | ||
*/ | ||
//============================================================================== | ||
|
||
#include <xrpl/protocol/Permissions.h> | ||
#include <xrpl/protocol/SField.h> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This and next include are not used. |
||
#include <xrpl/protocol/SOTemplate.h> | ||
#include <xrpl/protocol/jss.h> | ||
|
||
namespace ripple { | ||
|
||
Permission::Permission() | ||
{ | ||
granularPermissionMap = { | ||
{"TrustlineAuthorize", gpTrustlineAuthorize}, | ||
{"TrustlineFreeze", gpTrustlineFreeze}, | ||
{"TrustlineUnfreeze", gpTrustlineUnfreeze}, | ||
{"AccountDomainSet", gpAccountDomainSet}, | ||
{"AccountEmailHashSet", gpAccountEmailHashSet}, | ||
{"AccountMessageKeySet", gpAccountMessageKeySet}, | ||
{"AccountTransferRateSet", gpAccountTransferRateSet}, | ||
{"AccountTickSizeSet", gpAccountTickSizeSet}, | ||
{"PaymentMint", gpPaymentMint}, | ||
{"PaymentBurn", gpPaymentBurn}, | ||
{"MPTokenIssuanceLock", gpMPTokenIssuanceLock}, | ||
{"MPTokenIssuanceUnlock", gpMPTokenIssuanceUnlock}}; | ||
|
||
granularTxTypeMap = { | ||
{gpTrustlineAuthorize, ttTRUST_SET}, | ||
{gpTrustlineFreeze, ttTRUST_SET}, | ||
{gpTrustlineUnfreeze, ttTRUST_SET}, | ||
{gpAccountDomainSet, ttACCOUNT_SET}, | ||
{gpAccountEmailHashSet, ttACCOUNT_SET}, | ||
{gpAccountMessageKeySet, ttACCOUNT_SET}, | ||
{gpAccountTransferRateSet, ttACCOUNT_SET}, | ||
{gpAccountTickSizeSet, ttACCOUNT_SET}, | ||
{gpPaymentMint, ttPAYMENT}, | ||
{gpPaymentBurn, ttPAYMENT}, | ||
{gpMPTokenIssuanceLock, ttMPTOKEN_ISSUANCE_SET}, | ||
{gpMPTokenIssuanceUnlock, ttMPTOKEN_ISSUANCE_SET}}; | ||
} | ||
|
||
Permission const& | ||
Permission::getInstance() | ||
{ | ||
static Permission const instance; | ||
return instance; | ||
} | ||
|
||
std::optional<std::uint32_t> | ||
Permission::getGranularValue(std::string const& name) const | ||
{ | ||
auto const it = granularPermissionMap.find(name); | ||
if (it != granularPermissionMap.end()) | ||
return static_cast<uint32_t>(it->second); | ||
|
||
return std::nullopt; | ||
} | ||
|
||
std::optional<TxType> | ||
Permission::getGranularTxType(GranularPermissionType const& gpType) const | ||
{ | ||
auto const it = granularTxTypeMap.find(gpType); | ||
if (it != granularTxTypeMap.end()) | ||
return it->second; | ||
|
||
return std::nullopt; | ||
} | ||
|
||
bool | ||
Permission::isProhibited(std::string const& name) const | ||
{ | ||
// We do not allow delegating the following transaction permissions to other | ||
// accounts for security reason. | ||
if (name == "AccountSet" || name == "SetRegularKey" || | ||
name == "SignerListSet") | ||
return true; | ||
|
||
return false; | ||
} | ||
|
||
} // namespace ripple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
gp
prefix necessary? Can we just haveTrustlineAuthorize
, etc?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gp
is to indicate it is a granular permission set's member. I'm ok with either way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just deleted
gp