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

Feature/decouple metaptr logic #573

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

jjwoz
Copy link
Contributor

@jjwoz jjwoz commented Dec 6, 2021

Main goals were to separate logic out of each respected domain.

app/src/utils/utils.ts Outdated Show resolved Hide resolved
Comment on lines 484 to 489
export function assertIPFSPointer(logoPtr: MetaPtr | undefined) {
if (!logoPtr) throw new Error('assertIPFSPointer: logoPtr is undefined');
const protocol = BigNumber.from(logoPtr.protocol).toString();
if (!['0', '1'].includes(protocol))
throw new Error(`assertIPFSPointer: Expected protocol ID of 0 or 1, found ${protocol}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

assertIPFSPointer was intended an IPFS-specific method to ensure that if you are calling a method within the ipfs.ts file then the provided MetaPtr object must have a protocol ID of 1. Therefore IMO it doesn't make sense to move that out of ipfs.ts and into a generic utils file. Similarly, this method should not need to be aware of Protocol ID 0 (that was just temporary to get things working)

Gotta run to a meeting now and will finish reviewing later, but this hints to me that the PR may need some refactoring to fully decouple things

Copy link
Contributor

Choose a reason for hiding this comment

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

(you might already know that, which might be why this is still a draft PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You would be correct. I moved it to Utils because it doesn't really have anything to do with data specific needs like the rest of the functions in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mds1 I don't think we should hard code 1 or 0 - are we saying that for now we need to ensure it is equal to one? And if so we could possibly warrant a function that is solely used to check if the protocol number coming in, is acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry @jjwoz I'm not sure I follow your question here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this helps, but IMO the approach should be:

  • anything specific to IPFS (e.g. asserting something has a protocol ID of 1, or caching something to the ipfs- storage key, or taking a pointer that's known to have an ID of 1 and generating the URL) should live in ipfs.ts
  • anything generic (e.g. taking a pointer with an unknown protocol ID and determining it's URL) should live in the new metaPtrs.ts file, e.g. it seems to make sense to have the ptrToURI method live here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the reason I moved it because it is under the data folder - which I believe is surrounding async specific information. This is the basis for why I moved things around the way that I did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mds1 after standup today, I think the intent now for this PR will to be separate out by domain context (love it!) What I will probably start doing is:

  • Remove the data folder
  • organize by domain
  • start conversation and analyze to remove ambiguous utils files

@jjwoz jjwoz force-pushed the feature/decouple-metaptr-logic branch from 0f70afb to 95ed7e0 Compare December 9, 2021 13:50
@jjwoz jjwoz force-pushed the feature/decouple-metaptr-logic branch from 95ed7e0 to 5b4409d Compare December 14, 2021 15:52
…on that returns a boolean if it is found or not and added back ternary
@jjwoz jjwoz force-pushed the feature/decouple-metaptr-logic branch from 6c5d8eb to c834a5b Compare December 14, 2021 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants