API does not accept seed or public key for account #4404

drlongle · 2023-01-31T11:23:19Z

High Level Overview of Change

This PR resolves #3329, #3330 and #4337. Currently, seeds and public keys can be used in place of accounts at several locations in the API. This increases the API's complexity because the API needs to guess whether the input is a seed, a public key or an address. This PR prevents the API from accepting a seed or a public key as account ID. Since the API does not interpret account as seed, the option strict in several API methods is no longer needed and is removed.

Context of Change

Documentation needs to be updated.

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

src/ripple/rpc/handlers/AccountChannels.cpp

src/ripple/rpc/handlers/AccountLines.cpp

src/test/app/TxQ_test.cpp

seelabs

This looks good, thanks. I'm not sure I would have removed the public_key methods. That isn't a security concern, but I also understand the motivation - I don't object too strongly. I left a few suggestions. Nice job on this.

src/ripple/rpc/handlers/AccountChannels.cpp

src/ripple/rpc/handlers/AccountLines.cpp

src/ripple/rpc/handlers/AccountOffers.cpp

seelabs

👍 nice job on this.

gregtatcam · 2023-03-31T12:46:50Z

src/ripple/rpc/handlers/AccountChannels.cpp

+    if (!accountID)
+    {
+        rpcError(rpcACT_MALFORMED);
+    }


accountID is not really an optional. It's always seated if valid. Consider something like this:

auto const id = parseBase58<AccountID>(params[jss::account].asString()); if (!id) { rpcError(rpcACT_MALFORMED); } AccountID const accountID{std::move(id.value())};

It also results in fewer code changes. Other handlers have a similar code.

Thanks. Fixed.

gregtatcam · 2023-03-31T12:48:38Z

src/ripple/rpc/handlers/AccountChannels.cpp

-                     visitData.raDstAccount == (*sleCur)[sfDestination]))
+                    (*sleCur)[sfAccount] == *accountID &&
+                    (!visitData.raDstAccount ||
+                     *visitData.raDstAccount == (*sleCur)[sfDestination]))


Can simplify:

visitData.raDstAccount <= (*sleCur)[sfDestination])

Currently we check if DstAccount is provided or DstAccount == (*sleCur)[sfDestination].

Your suggestion seems to change the intention of this check. Would it work as expected?

You are right. I'm taking back this suggestion.

gregtatcam · 2023-03-31T12:56:34Z

src/ripple/rpc/handlers/AccountLines.cpp

-                            (!visitData.hasPeer ||
-                             visitData.raPeerAccount ==
+                            (!visitData.raPeerAccount ||
+                             *visitData.raPeerAccount ==


Can simplify: visitData.raPeerAccount <= line->getAccountIDPeer()

Currently, we check if peerAccount is not provided or peerAccount == line->getAccountIDPeer().

Your suggestion seems to change the intention of this check. Would it work as expected?

You are right. I'm taking back this suggestion.

gregtatcam · 2023-03-31T13:04:47Z

src/ripple/rpc/handlers/GatewayBalances.cpp

-                auto const id = parseBase58<AccountID>(j.asString());
-
-                if (id)
+                if (auto const id = parseBase58<AccountID>(j.asString()); id)


Don't need to check id.

This check currently exists. If we don't check, would we accept malformed accountID?

The original code is:

auto const id = parseBase58<AccountID>(j.asString()); if (id) { hotWallets.insert(*id); return true; }

In this case it does need to be checked. In case of if with initializer it's checked if there is no other condition.

intelliot · 2023-04-02T00:08:32Z

@drlongle it isn't necessary to rebase and force-push (though you can if you prefer - it's just a little trickier for reviewers to follow the changes). Using merge commits is fine if we're going to squash this PR when merging it.

intelliot · 2023-04-07T05:31:43Z

(Status: I believe drlongle still needs to respond to gregtatcam's suggestions)

gregtatcam · 2023-05-09T13:24:04Z

src/ripple/rpc/handlers/AccountCurrenciesHandler.cpp

-    AccountID accountID;  // out param
-    if (auto jvAccepted = RPC::accountFromString(accountID, strIdent, bStrict))
-        return jvAccepted;
+    auto const accountID = parseBase58<AccountID>(strIdent);


AccountID is not really optional. Please consider the updated suggested in AccountChannels.cpp comment. I will not repeat this comment for other files. All changed files under rpc/handlers have a similar code block: AccountInfo.cpp, AccountLines.cpp, AccountObjects.cpp, AccountOffers.cpp, DepositAuthorized.cpp, GatewayBalances.cpp, NoRipplecheck.cpp, OwnerInfo.cpp.

I have applied your suggested changes across the board. Let me know if you find any further issues.

gregtatcam

👍 LGTM

drlongle mentioned this pull request Jan 31, 2023

account_objects CLI usage statement is wrong (Version: 1.5.0) #3329

Closed

intelliot added API Change Medium Priority Will Need Documentation labels Feb 1, 2023

intelliot linked an issue Feb 2, 2023 that may be closed by this pull request

account_objects CLI usage statement is wrong (Version: 1.5.0) #3329

Closed

intelliot requested review from nbougalis, natenichols and seelabs February 2, 2023 22:50

intelliot removed Medium Priority labels Feb 7, 2023

intelliot mentioned this pull request Feb 14, 2023

API should never transparently interpret "account" as a seed (Version: 1.9.4) #4337

Closed

intelliot requested review from mDuo13 and justinr1234 February 14, 2023 03:32

justinr1234 reviewed Feb 20, 2023

View reviewed changes

src/ripple/rpc/handlers/AccountChannels.cpp Outdated Show resolved Hide resolved

src/ripple/rpc/handlers/AccountLines.cpp Show resolved Hide resolved

src/test/app/TxQ_test.cpp Outdated Show resolved Hide resolved

ckniffen mentioned this pull request Feb 21, 2023

Fixed NFTokenMinter field validation error in AccountSet Transaction XRPLF/xrpl.js#2201

Merged

7 tasks

drlongle changed the title ~~API does not interpret "account" as seed~~ API does not accept seed or public key for account Mar 1, 2023

intelliot modified the milestones: 1.10.1, 1.11.0 Mar 15, 2023

intelliot requested a review from gregtatcam March 22, 2023 17:58

intelliot assigned gregtatcam and seelabs Mar 29, 2023

seelabs reviewed Mar 29, 2023

View reviewed changes

src/ripple/rpc/handlers/AccountChannels.cpp Outdated Show resolved Hide resolved

src/ripple/rpc/handlers/AccountLines.cpp Outdated Show resolved Hide resolved

src/ripple/rpc/handlers/AccountOffers.cpp Outdated Show resolved Hide resolved

drlongle force-pushed the do-not-interpret-account-as-seed branch 2 times, most recently from 087a80f to ef7dae7 Compare March 30, 2023 16:31

seelabs approved these changes Mar 30, 2023

View reviewed changes

drlongle added 3 commits March 31, 2023 15:54

API does not interpret "account" as seed or public key

d183bf6

Fix clang-format

f028527

Use Account.human() instead of Account.id() in unit tests

b3cf64a

drlongle added 2 commits March 31, 2023 15:54

Drop non-strict account parsing XRPLF#3330

aa420cf

Do not default construct account ids

958a3e6

gregtatcam reviewed Mar 31, 2023

View reviewed changes

drlongle force-pushed the do-not-interpret-account-as-seed branch from ef7dae7 to 958a3e6 Compare March 31, 2023 16:48

intelliot requested review from gregtatcam and justinr1234 April 5, 2023 06:35

intelliot assigned justinr1234 and unassigned seelabs Apr 5, 2023

justinr1234 approved these changes Apr 5, 2023

View reviewed changes

intelliot linked an issue Apr 20, 2023 that may be closed by this pull request

API should never transparently interpret "account" as a seed (Version: 1.9.4) #4337

Closed

intelliot assigned drlongle and unassigned gregtatcam and justinr1234 Apr 20, 2023

drlongle added 2 commits May 9, 2023 10:41

Merge branch 'develop' into do-not-interpret-account-as-seed

0263106

Address reviewer's comments

4f970ef

drlongle closed this May 9, 2023

drlongle reopened this May 9, 2023

gregtatcam reviewed May 9, 2023

View reviewed changes

drlongle added 2 commits May 9, 2023 19:28

Address review comments

8997cb7

Address review comments

7d93021

gregtatcam approved these changes May 9, 2023

View reviewed changes

drlongle added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 15, 2023

intelliot merged commit 78076a6 into XRPLF:develop May 17, 2023

intelliot mentioned this pull request May 19, 2023

Drop non-strict account parsing #3330

Closed

This was referenced Jun 24, 2023

rippled 1.11.0 changes XRPLF/xrpl-dev-portal#1976

Merged

Remove obsolete owner_info API method #4594

Open

intelliot added the Added to API Changelog label Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API does not accept seed or public key for account #4404

API does not accept seed or public key for account #4404

drlongle commented Jan 31, 2023 •

edited

Loading

seelabs left a comment

seelabs left a comment

gregtatcam Mar 31, 2023

drlongle May 9, 2023

gregtatcam Mar 31, 2023

drlongle May 9, 2023

gregtatcam May 9, 2023

gregtatcam Mar 31, 2023

drlongle May 9, 2023

gregtatcam May 9, 2023

gregtatcam Mar 31, 2023

drlongle May 9, 2023

gregtatcam May 9, 2023

intelliot commented Apr 2, 2023

intelliot commented Apr 7, 2023 •

edited

Loading

gregtatcam May 9, 2023

drlongle May 9, 2023

gregtatcam left a comment

API does not accept seed or public key for account #4404

API does not accept seed or public key for account #4404

Conversation

drlongle commented Jan 31, 2023 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

seelabs left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot commented Apr 2, 2023

intelliot commented Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

drlongle commented Jan 31, 2023 •

edited

Loading

intelliot commented Apr 7, 2023 •

edited

Loading