Repository: dogecoin/dogecoin
File: src/wallet/rpcdump.cpp (C++)

Code before:
    if (!key.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Private key outside allowed range");

    CPubKey pubkey = key.GetPubKey();
    assert(key.VerifyPubKey(pubkey));
    CKeyID vchAddress = pubkey.GetID();
    {
        pwalletMain->MarkDirty();
        pwalletMain->SetAddressBook(vchAddress, strLabel, "receive");

        // Don't throw error in case a key is already there
        if (pwalletMain->HaveKey(vchAddress))
            return NullUniValue;

        pwalletMain->mapKeyMetadata[vchAddress].nCreateTime = 1;

        if (!pwalletMain->AddKeyPubKey(key, pubkey))
            throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");

        if (fRescan)
            attemptRescanFromHeight(request);
    }

    return NullUniValue;
}

void attemptRescanFromHeight(const JSONRPCRequest& request)
{

    CBlockIndex* pblockindex = chainActive.Genesis();

    if (request.params.size() > 3) {
        int nHeight = request.params[3].get_int();

        if (nHeight < 0 || nHeight > chainActive.Height())
            throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range");

        pblockindex = chainActive[nHeight];
    } else {
        // we have no implicit first height for a key, so we need to scan the whole chain
        pwalletMain->UpdateTimeFirstKey(1);
    }

    pwalletMain->ScanForWalletTransactions(pblockindex, true);
    pwalletMain->ReacceptWalletTransactions();
}

void ImportAddress(const CBitcoinAddress& address, const string& strLabel);
void ImportScript(const CScript& script, const string& strLabel, bool isRedeemScript)
{
    if (!isRedeemScript && ::IsMine(*pwalletMain, script) == ISMINE_SPENDABLE)
        throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script");

Review comment (refactor):
Good idea to generalize this into a utility function! I have some thoughts about the structure proposed but am open to counter-arguments, see last line:

1. I'm not sure about validating the request inside the helper function, especially because it only looks at a single parameter in a hardcoded position (`3`) and think it would be more readable to do the validation inside the controller (`ImportAddress`) and make `attemptRescanForHeight(request)` into a more definite `rescanFromHeight(int)`? That would make the controller decide what to do with user input and the rescan utility function just do what is decided?
2. To reduce code duplication we could make the height validation generic (`RpcValidateChainHeight(int)`)?

Right now, the remaining `importmulti` would also get a rescan height at position 3 (I'd guess), so ultimately I think the question is: **are we minimizing the impact of this change, or minimizing the impact of any future work?**

Code after:
    CKey key = vchSecret.GetKey();
    if (!key.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Private key outside allowed range");

    CPubKey pubkey = key.GetPubKey();
    assert(key.VerifyPubKey(pubkey));
    CKeyID vchAddress = pubkey.GetID();
    {
        pwalletMain->MarkDirty();
        pwalletMain->SetAddressBook(vchAddress, strLabel, "receive");

        // Don't throw error in case a key is already there
        if (pwalletMain->HaveKey(vchAddress))
            return NullUniValue;

        pwalletMain->mapKeyMetadata[vchAddress].nCreateTime = 1;

        if (!pwalletMain->AddKeyPubKey(key, pubkey))
            throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");

        if (fRescan) {
            const uint32_t nHeight = getHeightParamFromRequest(request, 3);
            attemptRescanFromHeight(nHeight);
        }
    }

    return NullUniValue;
}

uint32_t getHeightParamFromRequest(const JSONRPCRequest& request, const size_t pos)
{
    if (request.params.size() <= pos)
        return 1;

    const int nHeight = request.params[pos].get_int();

    if (nHeight < 0 || nHeight > chainActive.Height())
        throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range");

    return nHeight;
}

void attemptRescanFromHeight(const uint32_t nHeight)
{
    CBlockIndex* pblockindex = chainActive.Genesis();

    // we have no implicit first height for a key, so we need to scan the whole chain
    if (nHeight <= 1)
        pwalletMain->UpdateTimeFirstKey(1);
    else
        pblockindex = chainActive[nHeight];