refactor(experimental): add RPC methods returning just a number #1271

joncinque · 2023-04-19T23:54:54Z

Summary

While doing one of the simple RPC calls, I noticed there's a bunch of simple RPC calls that take just a commitment with optional minContextSlot, or nothing at all, and return either a number, or a number wrapped in an rpc response. The calls covered here are:

getFirstAvailableBlock (take nothing, just give number)
getMaxRetransmitSlot (ditto)
getMaxShredInsertSlot (ditto)
getSlot (takes commitment and context, just gives number)
getStakeMinimumDelegation (takes ONLY commitment, wraps the number)
getTransactionCount (takes commitment and context, just gives number)
minimumLedgerSlot (takes nothing, just gives a number)

Since so many of the tests just ended up doing the same as the getBlockHeight test, I moved them all into the same file. Then I noticed that some of them take parameters and some don't, and still kept them in the same file. Then I noticed some return just a number, and others wrap it, and still I kept them in the same file.

So if you want these broken up differently, I'm totally game! Also, I used the each construction, which I'm not sure if that's best or if you prefer something else.

Test Plan

pnpm test

Also, I ran prettier by hand with pnpm prettier --write '{,{src,test}/**/}*.{j,t}s', I didn't find any other scripts or configuration, and it seemed to pass the prettier test.

joncinque · 2023-04-20T00:01:19Z

Changing back to draft while I figure out the build error, sorry about that

joncinque · 2023-04-20T00:21:00Z

Ok all good, just had to use Reflect.get in the tests rather than obj[func]

steveluscher

I do indeed prefer the ‘repetitive but separate’ approach to tests, because:

reading through all the interpolation and metaprogramming in this PR's test makes me think too hard
if (when) someone breaks the RPC implementation for one of these suckers it's going to show up in a test called ‘getBigInt’ which will again require thinking
some of these methods are actually different (some return context, and others a raw value) so you have logic in this file that doesn't actually apply to all.
You could imagine adding more special cases here which would make these tests even more dissimilar. Could you actually start the validator with a custom minimum stake delegation and then add a test for that? Might you write a test that actually lands a transaction and asserts that the value of getTransactionCount() increased?
when we add a new RPC method that only returns an integer, it will be really obvious that the author didn't add a test if there's literally no file for it, but less obvious if it's just missed here. A 1:1 correspondence between a method definition and a test file makes it easy to spot.

I was going to say ‘despite all that I don't feel super strongly about this,’ but the more bullet points I added the more I think… yeah maybe I do :)

steveluscher · 2023-04-20T06:17:57Z

packages/rpc-core/src/rpc-methods/getFirstAvailableBlock.ts

@@ -0,0 +1,10 @@
+import { RpcResponse, Slot } from './common';
+
+type GetFirstAvailableBlockApiResponse = RpcResponse<Slot>;


This doesn't return an RpcResponse in fact. It's just the raw integer!

// Not an `RpcResponse<TValue>`... {context: {...}, value: 1234} // Actually just `TValue` 1234

Suggested change

type GetFirstAvailableBlockApiResponse = RpcResponse<Slot>;

type GetFirstAvailableBlockApiResponse = Slot;

Well at least I got it right in my PR description 😓 any idea about why this didn't get picked up by the compiler or the tests?

Edit: never mind, it makes sense that Reflect.get wouldn't be able to notice anything wrong since we're going around the compiler. Another point in favor of the separated tests!

Yeah! I mean we could write Typescript tests, but that’s pretty next level.

In case it wasn't clear what I meant by this, some people actually have written tests that assert whether source code raises type errors or not. Pretty extreme.

Haha don't worry, it was very clear. I didn't want to go there under any circumstances 😅

packages/rpc-core/src/rpc-methods/getMaxRetransmitSlot.ts

packages/rpc-core/src/rpc-methods/getMaxShredInsertSlot.ts

packages/rpc-core/src/rpc-methods/getSlot.ts

steveluscher · 2023-04-20T06:20:30Z

packages/rpc-core/src/rpc-methods/getStakeMinimumDelegation.ts

@@ -0,0 +1,14 @@
+import { RpcResponse, U64UnsafeBeyond2Pow53Minus1, Commitment } from './common';
+
+type GetStakeMinimumDelegationApiResponse = RpcResponse<U64UnsafeBeyond2Pow53Minus1>;


I think at this point it's worth creating a branded Lamports type. See Base58EncodedAddress for how to create a branded type.

This will imply that you can create a function that only accepts Lamports as a type and if you try to pass it a non-branded BigInt you'll get a Typescript error. Should save a couple million in mistake fees.

Yeah sure, where do you want it go? There's no clear place for it, so I'd probably just default to the top-level of rpc-core or in rpc-methods/common.ts but I'm not sure if it's an RPC-specific type

I have a @solana/keys package which is a nice place to put the address types, but for everything else I’ve just been hucking ‘primitive’ types into global typedefs.

Throw it anywhere for now, just try to avoid circular references between modules.

Ok, put it under common.ts

packages/rpc-core/src/rpc-methods/getTransactionCount.ts

packages/rpc-core/src/rpc-methods/minimumLedgerSlot.ts

joncinque · 2023-04-20T17:26:55Z

Ok, this should be ready for another look!

steveluscher

Thank you!!! I'll fix up my own nits and land this.

steveluscher · 2023-04-25T20:43:47Z

packages/rpc-core/src/rpc-methods/__tests__/get-first-available-block-test.ts

+        });
+    });
+    describe('when called with no parameters', () => {
+        it('returns a bigint', async () => {


I guess there are probably reasons why it's not safe to presume that the test validator will always return 0n here? I seem to remember that being a problem before: solana-labs/solana#23853

Similar to the stake delegation test, I didn't want this test to be in the business of validating the correctness of the response.

steveluscher · 2023-04-25T20:50:07Z

packages/rpc-core/src/rpc-methods/__tests__/get-stake-minimum-delegation-test.ts

+            it('returns the result as a bigint wrapped in an RpcResponse', async () => {
+                expect.assertions(1);
+                const result = await rpc.getStakeMinimumDelegation({ commitment }).send();
+                expect(result.value).toEqual(expect.any(BigInt));


Should we drop a TODO here to actually fetch the minimum delegation value from the stake program?

Probably not, now that I think about it. This test shouldn't really be in the business of testing the underlying method. It should stop short of testing the type of the responses, as you've done here.

Yeah exactly, that was my intention

steveluscher · 2023-04-25T20:51:02Z

packages/rpc-core/src/rpc-methods/common.ts

@@ -19,3 +19,5 @@ export type RpcResponse<TValue> = Readonly<{
    }>;
    value: TValue;
 }>;
+
+export type Lamports = U64UnsafeBeyond2Pow53Minus1 & { readonly __lamports: unique symbol };


Mmmm… I think this is more like…

Suggested change

export type Lamports = U64UnsafeBeyond2Pow53Minus1 & { readonly __lamports: unique symbol };

export type LamportsUnsafeBeyond2Pow53Minus1 = bigint & { readonly __lamports: unique symbol };

As you wish! In that case, do you also want to do SlotUnsafeBeyond2Pow53Minus1?

I asked how long it would take to get there and @willhickey told me ~114M years, so I think I decided against it.

Not opposed to it, for completeness, though.

ah gotcha, that definitely makes sense

steveluscher · 2023-04-25T20:53:43Z

packages/rpc-core/src/rpc-methods/minimumLedgerSlot.ts

+     * Returns the lowest slot that the node has information about in its ledger.
+     * This value may increase over time if the node is configured to purge older ledger data.
+     */
+    minimumLedgerSlot(): MinimumLedgerSlotApiResponse;


LOL @ no get.

github-actions · 2023-05-09T19:13:57Z

🎉 This PR is included in version 1.76.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2023-05-24T08:01:36Z

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

joncinque marked this pull request as draft April 20, 2023 00:01

joncinque force-pushed the expu64 branch from e95149f to beeb3ea Compare April 20, 2023 00:20

joncinque marked this pull request as ready for review April 20, 2023 00:20

refactor(experimental): add RPC methods returning a number

1cff51a

joncinque force-pushed the expu64 branch from beeb3ea to 1cff51a Compare April 20, 2023 00:32

steveluscher suggested changes Apr 20, 2023

View reviewed changes

joncinque added 3 commits April 20, 2023 14:27

refactor(experimental): fix RPC return types

768b289

refactor(experimental): split up tests into separate files

13ddd14

refactor(experimental): introduce branded Lamports and use it

e91062c

joncinque requested a review from steveluscher April 20, 2023 22:12

Merge branch 'master' into expu64

911a310

steveluscher approved these changes Apr 25, 2023

View reviewed changes

nits

acf72cb

steveluscher merged commit ed7c7b8 into solana-labs:master Apr 25, 2023

joncinque deleted the expu64 branch April 25, 2023 21:21

steveluscher added this to the web3.js Roadmap – April 2023 milestone May 9, 2023

github-actions bot added the released label May 9, 2023

github-actions bot locked as resolved and limited conversation to collaborators May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(experimental): add RPC methods returning just a number #1271

refactor(experimental): add RPC methods returning just a number #1271

joncinque commented Apr 19, 2023 •

edited

Loading

joncinque commented Apr 20, 2023

joncinque commented Apr 20, 2023

steveluscher left a comment

steveluscher Apr 20, 2023

joncinque Apr 20, 2023 •

edited

Loading

steveluscher Apr 20, 2023

steveluscher Apr 25, 2023

joncinque Apr 25, 2023

steveluscher Apr 20, 2023

steveluscher Apr 20, 2023

joncinque Apr 20, 2023 •

edited

Loading

steveluscher Apr 20, 2023

joncinque Apr 20, 2023

joncinque commented Apr 20, 2023

steveluscher left a comment

steveluscher Apr 25, 2023

joncinque Apr 25, 2023

steveluscher Apr 25, 2023

joncinque Apr 25, 2023

steveluscher Apr 25, 2023

joncinque Apr 25, 2023

steveluscher Apr 25, 2023

steveluscher Apr 25, 2023

joncinque Apr 25, 2023

steveluscher Apr 25, 2023

joncinque Apr 25, 2023

github-actions bot commented May 9, 2023

github-actions bot commented May 24, 2023

		@@ -0,0 +1,10 @@
		import { RpcResponse, Slot } from './common';

		type GetFirstAvailableBlockApiResponse = RpcResponse<Slot>;

		@@ -0,0 +1,14 @@
		import { RpcResponse, U64UnsafeBeyond2Pow53Minus1, Commitment } from './common';

		type GetStakeMinimumDelegationApiResponse = RpcResponse<U64UnsafeBeyond2Pow53Minus1>;

	export type Lamports = U64UnsafeBeyond2Pow53Minus1 & { readonly __lamports: unique symbol };
	export type LamportsUnsafeBeyond2Pow53Minus1 = bigint & { readonly __lamports: unique symbol };

refactor(experimental): add RPC methods returning just a number #1271

refactor(experimental): add RPC methods returning just a number #1271

Conversation

joncinque commented Apr 19, 2023 • edited Loading

Summary

Test Plan

joncinque commented Apr 20, 2023

joncinque commented Apr 20, 2023

steveluscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque Apr 20, 2023 • edited Loading

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

joncinque Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque commented Apr 20, 2023

steveluscher 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

github-actions bot commented May 9, 2023

github-actions bot commented May 24, 2023

joncinque commented Apr 19, 2023 •

edited

Loading

joncinque Apr 20, 2023 •

edited

Loading

joncinque Apr 20, 2023 •

edited

Loading