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

Implementing Transaction model #56

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Conversation

erdimaden
Copy link
Contributor

@erdimaden erdimaden commented Jun 5, 2024

What changed? Why?

  • Implementing new Transaction Model
  • Adding parseUnsignedPayload util function to parse the unsigned payloads

Qualified Impact

@erdimaden erdimaden changed the base branch from master to feat/base-mainnet-support June 5, 2024 18:23
@erdimaden erdimaden force-pushed the feat/transaction-model branch 3 times, most recently from 51749d4 to 0b0a36e Compare June 5, 2024 22:43
@erdimaden erdimaden marked this pull request as ready for review June 5, 2024 22:45
@erdimaden erdimaden force-pushed the feat/transaction-model branch from 0b0a36e to 85583cd Compare June 5, 2024 22:47
*/
constructor(model: TransactionModel) {
if (!model) {
throw new Error("Invalid model type");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it sufficient that this is typed via typescript? Or is this just guarding against a nil value?

Main reason we added that to the ruby client is for better error messaging when passing in the wrong object, because there is no typing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The guard in the constructor is mainly to catch null values and give better error messages, similar to what we did in the Ruby client. While TypeScript handles type checking, having this check is still useful for making the code more robust and the errors clearer. If someone uses the SDK in plain JavaScript, this kind of check is even more important since JavaScript doesn't have built-in type checking.

@erdimaden erdimaden requested a review from alex-stone June 5, 2024 23:13
Copy link
Contributor

@John-peterson-coinbase John-peterson-coinbase left a comment

Choose a reason for hiding this comment

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

Lets aim to be consistent with current implementation wrt documentation (Reference: https://github.com/coinbase/coinbase-sdk-nodejs/blob/master/src/coinbase/transfer.ts)

We can choose to do these as follow ups if necessary to unblock staking team, but I would like them to be fast followed before the release if possible!

cc: @alex-stone


describe("#unsignedPayload", () => {
it("returns the unsigned payload", () => {
const transaction = new Transaction(model);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move these out of the individual it blocks. Its not necessary to duplicate them if the model passed into the constructor is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense 👍

* @class
* @param model - The underlying Transaction object.
*/
constructor(model: TransactionModel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we use private constructor in most places. Is there a reason this diverges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some instances where we use constructors like User, Address, and FaucetTransaction classes. How do we determine which one to use? If we plan to use another method for initialization, we will essentially have the same guard function unless there is specific logic behind it, like in the Wallet class.

Comment on lines 53 to 54
/**
* Returns the status of the Transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* Returns the status of the Transaction.
/**
* Returns the Status of the Transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSDocs were generated using Ruby documentation, like the example below. @alex-stone you may want to update your PR too.

https://github.com/coinbase/coinbase-sdk-ruby/blob/master/lib/coinbase/transfer.rb#L134

}

/**
* Returns the from address for the Transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Returns the from address for the Transaction.
* Returns the From Address ID for the Transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* Returns the from address for the Transaction.
*
* @returns The from address
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns The from address
* @returns The From Address ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* Returns whether the Transaction is in a terminal state.
*
* @returns Whether the Transaction is in a terminal state
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns Whether the Transaction is in a terminal state
* @returns Whether the Transaction is in a terminal State

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* Returns the underlying raw transaction.
*
* @throws {InvalidUnsignedPayload} If the payload is invalid.
* @returns The raw transaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns The raw transaction
* @returns The ethers.js Transaction object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* Returns the underlying raw transaction.
*
* @throws {InvalidUnsignedPayload} If the payload is invalid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @throws {InvalidUnsignedPayload} If the payload is invalid.
* @throws {InvalidUnsignedPayload} If the Unsigned Payload is invalid.

@@ -102,3 +103,28 @@ export function destinationToAddressHexString(destination: Destination): string
throw new Error("Unsupported type");
}
}

/**
* Parses an unsigned payload and returns the JSON object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Parses an unsigned payload and returns the JSON object.
* Parses an Unsigned Payload and returns the JSON object.

* Parses an unsigned payload and returns the JSON object.
*
* @throws {InvalidUnsignedPayload} If the payload is invalid.
* @param payload - The unsigned payload.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param payload - The unsigned payload.
* @param payload - The Unsigned Payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

} as TransactionModel;
});

describe("#initialize", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

#initialize is a ruby construct, should this be just like describe('constructor', ....?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought we wanted to follow the same structure as our Ruby tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets keep the same structure but remain idiomatic to the language we are implementing in. initialize is ruby specific in this case. Transaction object is constructed with constructor in Typescript.

@erdimaden erdimaden force-pushed the feat/base-mainnet-support branch from 366d5a0 to 7001562 Compare June 6, 2024 01:42
@erdimaden erdimaden force-pushed the feat/transaction-model branch from 85583cd to ca96717 Compare June 6, 2024 05:59
@erdimaden erdimaden force-pushed the feat/base-mainnet-support branch 3 times, most recently from 3bbec8c to bbca705 Compare June 6, 2024 16:59
import { Transaction as TransactionModel } from "../../client/api";
import { Transaction } from "./../transaction";

describe("Coinbase::Transaction", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe("Coinbase::Transaction", () => {
describe("Transaction", () => {

It seems from other projects using just class name is most idiomatic.
Lets clean these up in a follow up in other files to use ClassName to describe top level block.
#InstanceMethodName to describe instance methods and .staticMethodName to describe static methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update this we will have the most of test case refactoring/changes with [PSDK-245]

@erdimaden erdimaden force-pushed the feat/base-mainnet-support branch from bbca705 to 1e5d379 Compare June 6, 2024 17:07
@erdimaden erdimaden force-pushed the feat/transaction-model branch from 24b84c6 to 8d3e4e9 Compare June 6, 2024 17:09
@erdimaden erdimaden changed the base branch from feat/base-mainnet-support to release-v0.0.7 June 6, 2024 17:21
Comment on lines -5 to 10
### Added

- Added Base Mainnet network support

### Changed
Updated the usage of `Coinbase.networkList` to `Coinbase.networks`

- Updated the usage of `Coinbase.networkList` to `Coinbase.networks`
### Added
- Added Base Mainnet network support

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets format the CHANGELOG.md before merge

Copy link
Contributor

Choose a reason for hiding this comment

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

We also should add the transaction class addition to change log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, just merged but I'm going to format in my PR

Copy link
Contributor

Choose a reason for hiding this comment

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

no worries. Lets just change before the release.

@erdimaden erdimaden merged commit 10f3860 into release-v0.0.7 Jun 6, 2024
4 checks passed
@erdimaden erdimaden deleted the feat/transaction-model branch June 21, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants